Update modals #275
Update modals #275
Conversation
|
While you are poking around could I request adding a boolean isDisabled to the |
Consumers who want it can still add it with the styled-system overflowY prop.
Because FLCOM unconditionally adds `padding: 1em 0;` to all <p> tags.
|
Looks like the DefaultModalFooter already supports disabling the buttons with a prop, we just don't call it out on the propTypes for the Modal. I'll add it in this PR. |
Still a work in progress, this component is actually identical to the v5 LegacyModal.
RobertBolender
force-pushed
the
update-modals
branch
from
faeb5ef
to
4e7ca47
Compare
Also moved the file to the shared-hooks directory.
components/modal/modal.jsx
Outdated
| onClose, | ||
| children, | ||
| theme, | ||
| styleOverrides, |
There was a problem hiding this comment.
Is this the v6 API? We should remove this prop, if so.
There was a problem hiding this comment.
Looks like we were only using it for zIndex, which is available as a styled-system prop. I'll add a deprecation notice.
There was a problem hiding this comment.
Oh, we had been passing the zIndex style-override to the backdrop, which we are otherwise not exposing to the consumer.
How would you feel about adding an explicit zIndex prop to the Modal that would be passed to the Backdrop instead? I can't think of a scenario where you would want to set the zIndex on the Modal itself in a way that wouldn't affect the Backdrop.
components/utils/deprecate.js
Outdated
| @@ -0,0 +1,30 @@ | |||
| import React from 'react'; | |||
|
|
|||
| var hits = new Set(); | |||
There was a problem hiding this comment.
Still have that var :-)
We should figure out why this is allowed.
There was a problem hiding this comment.
eslint-config-faithlife has prefer-const enabled but not no-var
https://eslint.org/docs/rules/no-var#require-let-or-const-instead-of-var-no-var
This should result in fewer surprises for consumers. Focus styles for input elements should not be cut off from the Modal.Content, and overflow content should scroll when expected to.
RobertBolender
force-pushed
the
update-modals
branch
from
a9645d9
to
b977a4f
Compare
React SSR does not support hydrating Portals, so when we create a Portal we do so within a useEffect that will only run in the browser. Functionality isn't changing in this commit, we're just making the code a little clearer.
Spec:
Resolves #265
Consumers can opt-in to the new v6 Modal API by importing from:
import { Modal } from @faithlife/styled-ui/v6Deprecation warnings have been added to inform users of the new API. The /v6 entrypoint will continue to be supported (with a deprecation warning) throughout the lifetime of v6 and won't be removed until v7 is released. This will ensure plenty of time for a smooth transition.
BREAKING CHANGES:
ModalContent no longer hasoverflow-y: autoby default. Consumers who want their modal content to scroll should add it themselves.<ModalContent>accepts aoverflowYstyled-system prop.This decision has been reversed.
ModalHeader no longer has a bottom-border.
ModalContent no longer has vertical padding by default.Modals that are rendered without a ModalFooter may need to specify bottom padding on their ModalContent, unless they don't want it (such as for the fullscreen SmartMediaEditor, for example).Removed the unused
tabindexoption from modal footer buttons. (zero active uses at this time)