docs(Expandable section, file upload, modal, pagination): add compone… #7979
Conversation
|
Preview: https://patternfly-react-pr-7979.surge.sh A11y report: https://patternfly-react-pr-7979-a11y.surge.sh |
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
By reading these descriptions of ModalBoxXX components, It's feeling like we shouldn't be exporting the ModalBoxXX components? they are used by the ModalContent component. That component likely shouldn't be exported either, since its imported by the Modal component and not used in our examples?
Ceasing to export these sub-components would definitely be a breaking change, but we may not need to reference them in the docs if they are meant to be internal components only? Either that, or we should open an issue to build an example demonstrating how consumers can use these exported components. @tlabaj do you have more insights or a preference here?
| import topSpacer from '@patternfly/react-tokens/dist/esm/c_modal_box_m_align_top_spacer'; | ||
|
|
||
| /** Acts as a wrapper that contains only the modal content. */ | ||
|
|
There was a problem hiding this comment.
Do any of our examples demonstrate using this component? I don't think this comment is clear enough about why Id want or need to use a ModalBox. Does it change the layout or padding of the modal content? How would I pass this to the Modal?
There was a problem hiding this comment.
I'm wondering, if this is really meant to be an internal components and not something exposed in our docs...
There was a problem hiding this comment.
@thatblindgeye @nicole The Modal sub components were never design to be exported initially. Someone exported them at some point. We have an issue for the major release to make the Modal composable. I would actually hold of on documenting the modal until that is implemented.
| import { css } from '@patternfly/react-styles'; | ||
| import styles from '@patternfly/react-styles/css/components/ModalBox/modal-box'; | ||
|
|
||
| /** The main body content of a modal. Depending on the context, this sub-component may be optional. */ |
There was a problem hiding this comment.
How is this related to a ModalBox? it's name implies that it's a child of ModalBox? their relationship an and the context during which this might be desired should be clearer if possible.
There was a problem hiding this comment.
I'm wondering, if this is really meant to be an internal components and not something exposed in our docs...
thatblindgeye
force-pushed
the
subcomponent_desc_2
branch
from
ffd6f3e to
d67b0b9
Compare
|
@tlabaj @nicolethoen The latest update removes descriptions to the Modal components, and also removes all but Modal from the prop docs. |
4 participants
…nt descriptions
What: Closes #7978
I also reordered props alphabetically to make it easier to parse (most files updated involve just this), and did some minor tidying of prop descriptions.
Currently only some of the Modal sub-components are exposed on the Modal page. I'm wondering whether any others should be as well, or if any should be removed from the Modal page.
Additional issues: