feat(Modal): add prop to toggle modal position to scrollable/fixed #377
Conversation
src/components/Modal/Modal.scss
Outdated
| &__overlay { | ||
| position: fixed; | ||
| top: 0; | ||
| left: 0; | ||
| right: 0; | ||
| bottom: 0; | ||
|
|
||
| width: 100%; | ||
| height: 100%; |
There was a problem hiding this comment.
All the sizes of the other elements are in vh/vw units, here as well width: 100vw; height: 100vh; can be used for consistency
| overlayClassName?: string; | ||
| /** Additional class to be applied to the portal */ | ||
| portalClassName?: string; | ||
| } |
src/components/Modal/Modal.scss
Outdated
| @@ -1,10 +1,19 @@ | |||
| .Modal { | |||
| &__body--open { | |||
There was a problem hiding this comment.
Having element body with modifier open doesn't make much sense because if it is in a DOM tree, it is already open (in other words such thing as body--closed doesn't exists).
So, can be just body.
src/components/Modal/Modal.tsx
Outdated
| @@ -19,6 +45,7 @@ export const Modal = (props) => { | |||
| } = props; | |||
|
|
|||
| const { className: portalClass } = block({ className: portalClassName, ...rest }); | |||
| const { className: bodyOpenClass } = elem('body--open', props); | |||
There was a problem hiding this comment.
Here, if we'd need to have a class name body--open, that proper way to achieve it would be:
elem('body', { ...props, open: true });
But since we don't need open modifier at all, it can be just elem('body', props);
| @@ -29,12 +56,24 @@ export const Modal = (props) => { | |||
| const { className: contentEnteredClass } = elem('content--entered', props); | |||
| const { className: contentExitedClass } = elem('content--exited', props); | |||
There was a problem hiding this comment.
Ah, I believe you copypasted elem('body--open', props); from here, bu these are examples of not correct usage of elem function.
Should be:
const { className: contentEnteredClass } = elem('content', { ...props, entered: true });
const { className: contentExitedClass } = elem('content', { ...props, exited: true });There was a problem hiding this comment.
Because the way this is used in React Modal, setting it in this way makes final DOM having repeated classes, that looks messy. So I kept it as it is now. Alternatively we could rename the enter/exit classes in the scss files so that they don't use the usual modifier patterns to avoid internal confusion, e.g. &__enteredContent instead of &__content--entered.
There was a problem hiding this comment.
Ah, I see. Now I remember there was a task to cover this case in bem. Ok, let's keep it this way then so far.
Story: JF-2367
Checklist