Convert FileSetModel modal to a Dialog component #4041
Conversation
|
@bmquinn - The close "X" is no longer getting cut off for me. I did notice that if I actually click it to close after I've already typed in values in the form, they are not getting cleared. Should those things be happening? For the straight shot add/replace w/out any mistakes on the users part everything seems fine. |
…S3ObjectPicker component
|
@bmquinn - the form field data clearing seems fixed now, but for some reason now there is an extra "Drag 'n' drop a file here, or click to select file" below the option 2, which wasn't there before (and you can't drag and drop there...) Also, I'm still seeing the "Accepted files Uploading NaN undefined" error that you put a screenshot of in the description if I click on a file name in S3 and then end up canceling or closing. And then try again. 
|
There was a problem hiding this comment.
This seems much better with the Radix Dialog! I forgot we had introduced that as a component in Meadow.
Take it or leave it but one thing I noted is that we could use the disabled attribute on the button so that the footer is visible at all times, subtly indicating to the user that they have not completed the form yet. My Meadow is being goofy right now and not updating changes in the browser so I couldn't completely verify, but this should be close:
| {s3UploadLocation && ( | ||
| <> | ||
| <Button | ||
| isText | ||
| type="button" | ||
| onClick={handleCancel} | ||
| data-testid="cancel-button" | ||
| > | ||
| Cancel | ||
| </Button> | ||
| <Button isPrimary type="submit" data-testid="submit-button"> | ||
| Ingest fileset | ||
| </Button> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
| {s3UploadLocation && ( | |
| <> | |
| <Button | |
| isText | |
| type="button" | |
| onClick={handleCancel} | |
| data-testid="cancel-button" | |
| > | |
| Cancel | |
| </Button> | |
| <Button isPrimary type="submit" data-testid="submit-button"> | |
| Ingest fileset | |
| </Button> | |
| </> | |
| )} | |
| <Button | |
| isText | |
| type="button" | |
| onClick={handleCancel} | |
| data-testid="cancel-button" | |
| > | |
| Cancel | |
| </Button> | |
| <Button | |
| disabled={!s3UploadLocation} | |
| isPrimary | |
| type="submit" | |
| data-testid="submit-button" | |
| > | |
| Ingest fileset | |
| </Button> |
| )} | ||
| </section> | ||
|
|
||
| <footer className="modal-card-foot is-justify-content-flex-end"> |
There was a problem hiding this comment.
@bmquinn I'd suggest losing the Bulma modal- specific className (modal-card-foot) on the footer component. And any other modal- specific classes if they're hanging around the new Dialog implementation, as they may depend upon sibling elements to work properly. And for multiple eyes inspecting the code moving forward, it's just a clean break of that style dependency.
|
Hi @mathewjordan and @adamjarling, just pushed up a commit that addresses your comments. Made a screencast to show the new s3picker.mov |
There was a problem hiding this comment.
@bmquinn Sorry for the nitpicky updates, but it's such good work, might as well tighten it up just a tiny bit more.
In addition to the suggestions below, I'd update this style to add a bit of breathing room to the DialogTitle via paddingBottom. Maybe something like:
meadow/app/assets/js/components/UI/Dialog/Dialog.styled.jsx
Lines 42 to 44 in 0e0a011
const DialogTitle = styled(Dialog.Title, {
fontWeight: "700",
paddingBottom: "1rem",
});| @@ -43,10 +43,20 @@ const DialogTitle = styled(Dialog.Title, { | |||
| fontWeight: "700", | |||
| }); | |||
|
|
|||
| const DialogFooter = styled(Dialog.Footer, { | |||
There was a problem hiding this comment.
| const DialogFooter = styled(Dialog.Footer, { | |
| const DialogFooter = styled("footer", { |
Dialog.Footer is not an export from Radix UI's Dialog (https://www.radix-ui.com/primitives/docs/components/dialog#anatomy), so you might run into a compilation error at worst, warning at best.
| display: "flex", | ||
| justifyContent: "flex-end", | ||
| padding: "1rem", | ||
| borderTop: "1px solid #e5e7eb", |
There was a problem hiding this comment.
| borderTop: "1px solid #e5e7eb", |
| justifyContent: "flex-end", | ||
| padding: "1rem", | ||
| borderTop: "1px solid #e5e7eb", | ||
| marginTop: "auto", // Pushes the footer to the bottom of the dialog |
There was a problem hiding this comment.
| marginTop: "auto", // Pushes the footer to the bottom of the dialog | |
| marginTop: "1rem", |
Summary
The modal window is currently being vertically cut off after a certain height, obscuring the "close" button. This PR converts the
FileSetModalto aDialogcomponent which does a better job of handling the z-index and scrolling issues.Also fixes a bug with the S3ObjectPicker when selecting an S3 object from the list (the behavior in the screenshot is visible on Meadow staging now, this commit makes it go away 😄 )
Specific Changes in this PR
FileSetModal.jsxS3ObjectPickercomponent (see screenshot above)Version bump required by the PR
See Semantic Versioning 2.0.0 for help discerning which is required.
Steps to Test
Dialogcomponent is working properly when you hit the "Add a File Set" buttonAlso please let developers know if there are any special instructions to test this in the development environment.
🚀 Deployment Notes
Note - if you check any of these boxes go to the (always open)
main<-stagingPR and add detailed notes and instructions to help out others who may end up deploying your changes to productionmix meadow.pipeline.setuprun)miscellanyTested/Verified