Skip to content

Conversation

@ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Aug 29, 2024

What are the relevant tickets?

This is a followup to #1463

Description (What does it do?)

This does two things:

  1. Fixes some spacing issues... Notably the bottom of the dialog
  2. Fixes a bug where:
  3. Open the add to list dialog. ..... call this X
  4. select some checkboxes .... call this Y
  5. do not save. Close the dialog
  6. re-open ... you get Y not X

Screenshots (if appropriate):

Before / After

Screenshot 2024-08-29 at 1 37 38 PM Screenshot 2024-08-29 at 1 37 15 PM

How can this be tested?

  1. View other dialogs. They should in general be unchanged (with the exception of the 4px gap mentioned below).
  2. View the add to list dialog. Try to repeat bug described above. You should get the initial state ("X")

@ChristopherChudzicki ChristopherChudzicki force-pushed the cc/dialog-spacing-and-reset branch from bfc75c0 to d14b551 Compare August 29, 2024 17:42
Comment on lines +185 to +194
use: {
loader: "swc-loader",
options: {
jsc: {
experimental: {
plugins: [["@swc/plugin-emotion", {}]],
},
},
},
},
Copy link
Contributor Author

@ChristopherChudzicki ChristopherChudzicki Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is https://www.npmjs.com/package/@swc/plugin-emotion

In particular, in dev mode, this gives us more useful class names. (production is unchanged).

Added in this PR in the course of trying to figure out where a style was coming from.

Comment on lines -38 to 39
alignItems: "start",
"> *": {
"> *:not(:last-child)": {
marginBottom: "4px",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule is supposed to make the 4px gap shown below. Note that there is no gap below the last child.

Figma:

Screenshot 2024-08-29 at 1 28 06 PM

@ChristopherChudzicki ChristopherChudzicki changed the title Cc/dialog spacing and reset Fix dialog spacing + reset AddToListDialog Aug 29, 2024
Comment on lines -63 to -66
/**
* MUI Dialog's [TransitionProps](https://mui.com/material-ui/api/dialog/#props)
*/
TransitionProps?: DialogProps["TransitionProps"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been removed when we made the dialogs "Slide down from top".

@ChristopherChudzicki ChristopherChudzicki added the Needs Review An open Pull Request that is ready for review label Aug 29, 2024
Comment on lines -144 to -145
{children}
{footerContent}
Copy link
Contributor Author

@ChristopherChudzicki ChristopherChudzicki Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that footerContent was used for can just be passed as part of the children.

(In a previous design, it needed to be separate)

@gumaerc gumaerc self-assigned this Aug 29, 2024
Comment on lines +186 to +190
{mutation.isError && !formik.isSubmitting && (
<Alert severity="error">
There was a problem saving your list. Please try again later.
</Alert>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what it looks like now. Same as before.

Screenshot 2024-08-29 at 1 52 42 PM

Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


return (
<Dialog
<FormDialog
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had considered using FormDialog, but at first it didn't seem necessary since this is barely a form and there's only one input element. This makes sense though, it cleans things up and onReset fixes our issue with the selections persisting.

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review August 29, 2024 18:07
@ChristopherChudzicki ChristopherChudzicki merged commit fcdb384 into main Aug 29, 2024
This was referenced Aug 29, 2024
@rhysyngsun rhysyngsun deleted the cc/dialog-spacing-and-reset branch February 7, 2025 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants