chore(DualListSelector/MultipleFileUpload): add composable structure and component descriptions #7603
Conversation
|
Preview: https://patternfly-react-pr-7603.surge.sh A11y report: https://patternfly-react-pr-7603-a11y.surge.sh |
thatblindgeye
force-pushed
the
composable_structure
branch
2 times, most recently
from
64a9949 to
ca0147b
Compare
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
I'm really liking this! I just have a couple small notes.
|
|
||
| /** Acts as a container for all other MultipleFileUpload sub-components. | ||
| * Logic for the callback that gets called when a file is uploaded can also | ||
| * be passed into this sub-component. |
There was a problem hiding this comment.
Nit: I would maybe reword the second sentence a bit to say something about this sub-component providing the actual file uploading functionality of the component, rather than just saying it's the component that would take the file uploading callback.
There was a problem hiding this comment.
What about something along the lines of "This sub-component provides the functionality for file uploads, and logic for the callback that gets called when a file is uploaded can also be passed in."
There was a problem hiding this comment.
Maybe a slight tweak to something like "This sub-component provides the functionality for file uploads, and access to the uploaded files via a callback." or something like that?
There's something about the "logic for the callback..." that feels off to me, though it could totally just be in how I'm reading it.
There was a problem hiding this comment.
Perhaps it makes things a little too verbose. I like your suggestion as it more succinctly describes what's going on.
| /** Renders a progress bar for each individual file that has been attempted to be uploaded, | ||
| * including the file name, file type, file size, and upload status. Each status item also | ||
| * has a "remove" button to remove individual items from the status list. | ||
| */ |
There was a problem hiding this comment.
Nit: I'm not sure quite how technical we really need to be here, but it might be good if we mention that this sub-component also can handle the reading of the file that it gets passed?
There was a problem hiding this comment.
Maybe something like, "Renders a progress bar for each individual file that has been attempted to be uploaded. This sub-component also handles reading the uploaded files to render the file name, file type, file size, and upload status." (may want to separate "upload status" from the rest? not sure if that would really tie into the file getting read or not)
There was a problem hiding this comment.
I like that! The only reservation I have about it is that I feel like (maybe) it implies that having it read the files is mandatory rather than something consumers can opt out of if they need custom file reading logic?
Again I might totally just be reading this wrong or reading into it too much 😅
thatblindgeye
force-pushed
the
composable_structure
branch
from
b5e59f2 to
96d7a23
Compare
thatblindgeye
changed the title
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
…and component descriptions (patternfly#7603) * chore(docs): create composable structure template * Move component descriptions to individual files * Add composable structure info to dual list selector * Add description to tree sub-component * Verbiage changes to sub-component descriptions
4 participants
What: Closes #7691, closes #7692 This is a proposed template for composable components, which includes the general structure and purpose of sub-components.
Dual list selector
File upload - multiple
The two components updated in this PR show how two of the different composable components could look. One being a component that is only composable (file upload multi) or one that has a non-composable and a composable structure (dual list selector).
For the dual list selector, I created a new level 2 heading for the "Composable structure" section to separate it out a little more. I'm not sure if this would end up causing any confusion, i.e. "is the Examples section the only examples, why does this other section also have examples then, etc etc". I also did notice a typo in my description for DualListSelectorControl (I only mention "selected options" when it should be "selected or all") that I will update along with any other suggestions that may be made.
Additional issues: