Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [M3-7117] - Fix style consistency issues with NodeBalancer create flow panels #9673

Merged
merged 7 commits into from
Sep 15, 2023

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Sep 13, 2023

Description 📝

This PR fixes the alignment of the NB create flow panels and accordions. This one has bothered me for a while!

Preview 📷

Before After
Screenshot 2023-09-13 at 13 30 52 Screenshot 2023-09-13 at 14 07 00

How to test 🧪

  1. Navigate to nodebalancers/create
  2. Check the alignment of all visible panels on desktop and mobile
  3. Check the alignment of all conditionally rendered panels on desktop and mobile (toggle Protocol in Configurations & Type in Active Health Checks etc)

@abailly-akamai abailly-akamai self-assigned this Sep 13, 2023
@abailly-akamai abailly-akamai marked this pull request as ready for review September 13, 2023 18:10
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

looks much better! Thanks!

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

I know it's like this in prod now, but have certs and keys always been this way for HTTPS? It looks squashed.

Screenshot 2023-09-13 at 12 50 32 PM

packages/manager/.changeset/pr-9673-fixed-1694628742725.md Outdated Show resolved Hide resolved
@mjac0bs mjac0bs changed the title fix: [fix-M3-7117] fix style consistency issues with NB create flow panels fix: [M3-7117] - Fix style consistency issues with NodeBalancer create flow panels Sep 13, 2023
@carrillo-erik
Copy link
Contributor

This is just a UX observation and not related to your work on this PR. It does appear like we have a lot of empty space to the right of the red line in the below. We can probably utilize that extra space more efficiently and the helper text below Proxy Protocol and other components would not look so smushed together.
Screenshot 2023-09-14 at 8 12 31 AM

@cpathipa cpathipa added the Approved Multiple approvals and ready to merge! label Sep 14, 2023
@abailly-akamai
Copy link
Contributor Author

abailly-akamai commented Sep 15, 2023

@carrillo-erik fair enough! i adjusted things slightly

Screenshot 2023-09-14 at 20 19 08

@@ -225,8 +225,8 @@ export const NodeBalancerConfigPanel = (

{protocol === 'https' && (
<Grid xs={12}>
<Grid container spacing={2}>
<Grid xs={12}>
<Grid columns={12} container spacing={2}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaalah wanted to grab your attention about this particular issue with the conditional rendering of an instance of <Grid container /> (Unstable_Grid2) which does not seem to load the CSS grid column variables unless the component is initially rendered on page load.

https://stackoverflow.com/questions/75546810/mui-system-grid-columns-css-var-is-undefined

Copy link
Contributor

@jaalah-akamai jaalah-akamai Sep 15, 2023

Choose a reason for hiding this comment

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

@abailly-akamai A nested container should be direct child of another grid container, unless you explicitly want the grid container to be a new root container with it's own variable scope. So what you want here is to remove <Grid xs={12}> and just do <Grid columns={12} container spacing={2} xs={12}>

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that the problem? why would I need to redefine the columns count?

Copy link
Contributor

@jaalah-akamai jaalah-akamai Sep 15, 2023

Choose a reason for hiding this comment

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

In case you wanted to change the columns count for whatever reason: https://mui.com/material-ui/react-grid2/#nested-grid. If you wanted a different size grid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right right, it's just not needed. the default is 12. lemme make quick changes

@mjac0bs
Copy link
Contributor

mjac0bs commented Sep 15, 2023

Wow, what a form this is. Thanks for unsquishing the key and cert fields.

I found a couple of other weird things if you'd like to address them in this PR, but don't want to hold this up and prevent it from getting it into the release because your other changes are definitely an improvement upon what's in prod.

Screenshot 2023-09-15 at 7 57 59 AM

Screenshot 2023-09-15 at 7 55 58 AM

@abailly-akamai
Copy link
Contributor Author

@mjac0bs will fix those in a new PR

@abailly-akamai
Copy link
Contributor Author

@mjac0bs @jaalah-akamai actually addressed all comments. good to go!

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Great, thanks!

I'm going to make a follow up ticket (M3-7142) for some copy writing here. Just noticed our sentence says "Roundrobin." which is not entirely helpful, but I'm not sure what actually goes here/how long it's been like that. And whether we want to continue to display the description of every algorithm type, or just the one that is currently selected.
Screenshot 2023-09-15 at 9 48 10 AM

@jaalah-akamai jaalah-akamai merged commit 3c5d91d into linode:develop Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants