feat(Modal): css and html structure improvements, add footer shadow for layouts #2568
Conversation
packages/core/src/components/ModalNew/ModalContent/__tests__/ModalContent.test.tsx
Outdated
Show resolved
Hide resolved
packages/core/src/components/ModalNew/ModalTopActions/ModalTopActions.module.scss
Outdated
Show resolved
Hide resolved
...core/src/components/ModalNew/layouts/ModalSideBySideLayout/ModalSideBySideLayout.module.scss
Outdated
Show resolved
Hide resolved
| overflow: hidden; | ||
| position: relative; | ||
|
|
||
| > * { |
There was a problem hiding this comment.
This is usually bad for performance as css selectors are read from right to left. Any way to avoid it?
There was a problem hiding this comment.
I can just target img and video tags instead
There was a problem hiding this comment.
I'm not sure how Lottie would behave anyway with the current CSS I applied
There was a problem hiding this comment.
We should probably have Lottie as a dev dep for this kind of cases
There was a problem hiding this comment.
Dev dep? Why? Plus I feel like that this css is creating a side effect for the users, don't you think? Because they can provide an img element and use some css on that element but you will most likely override their css
There was a problem hiding this comment.
Their css should "win"
I'm using 0,1,0 specificity
so if they override it, their css would be the one rendered
also, you suggested in a different PR to apply a predefined css
otherwise the ModalMedia is redundant as ModalContent
There was a problem hiding this comment.
I meant predefined css for an image that you provided by a prop, not by children, but this was dropped, no?
But anyway, I mean that targeting elements inside your component is creating a side effect. I don't think that the css is bad, it just should only affect the elements inside your component and not elements provided by the user. Plus you're applying style that may not fit to all elements like object-fit. I think the best combination would be to have an Image/Video components or maybe Media. This along with the ModalMedia will get along well. But probably out of this scope
There was a problem hiding this comment.
I need to release the layouts with a solution for that
I cannot leave it with every user doing whatever they want, it would create inconsistency
let's have a talk about it tomorrow
There was a problem hiding this comment.
I've removed the entire code that's affecting the users
I think each case should be examined differently by the user implementing and each implementation would require a bit different css handling
Thanks!
packages/core/src/components/ModalNew/layouts/ModalBasicLayout/ModalBasicLayout.tsx
Show resolved
Hide resolved
| /** | ||
| * The footer is optional, so we need to check if it exists before applying padding. | ||
| * If it exists (or "has" not supported), we apply padding to the content, if it doesn't, we don't apply padding. | ||
| */ |
There was a problem hiding this comment.
Why not use context? Or data attrs?
There was a problem hiding this comment.
this would force the footer to access the context
I wanted to avoid it
but you're basically right, it's probably a better solution
There was a problem hiding this comment.
I've added the padding to always be visible, as the footer shouldn't be omit
…sing z-index 7794144599
YossiSaadi
merged commit c5a54da
into
feat/yossi/new-modal-layouts
YossiSaadi
deleted the
feat/yossi/new-modal-layouts--css-improvements-7741641864-and-footer-shadow-logic-7719009083
branch
https://monday.monday.com/boards/3532714909/pulses/7741641864
https://monday.monday.com/boards/3532714909/pulses/7719009083