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-6931] - Fixed top spacing for NodeBalancer empty state #9746

Merged
merged 2 commits into from
Oct 6, 2023
Merged

fix: [M3-6931] - Fixed top spacing for NodeBalancer empty state #9746

merged 2 commits into from
Oct 6, 2023

Conversation

tyler-akamai
Copy link
Contributor

@tyler-akamai tyler-akamai commented Oct 2, 2023

Description 📝

Fixed the top spacing for the NodeBalancer empty state

Major Changes 🔄

  • Added top padding to NodeBalancer empty state

Preview 📷

Before
Domains NodeBalancer
Screenshot 2023-10-02 at 3 21 57 PM Screenshot 2023-10-02 at 3 22 22 PM
After
Domains NodeBalancer
Screenshot 2023-10-02 at 3 20 07 PM Screenshot 2023-10-02 at 3 20 30 PM

How to test 🧪

  1. Turn on MSW and ensure there are 0 active NodeBalancers
  2. Navigate to http://localhost:3000/nodebalancers
  3. Ensure the spacing is consistent with the other empty state pages (ex. Volumes, Domains, Kubernetes, etc.)

@tyler-akamai tyler-akamai requested a review from a team as a code owner October 2, 2023 19:23
@tyler-akamai tyler-akamai self-assigned this Oct 2, 2023
@tyler-akamai tyler-akamai requested review from cpathipa and cliu-akamai and removed request for a team October 2, 2023 19:23
Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @tyler-akamai!

Not to suggest increasing the scope of this work, but is there a technical reason why this is the only empty landing page that doesn't use the <ResourcesSection /> component?

@tyler-akamai
Copy link
Contributor Author

tyler-akamai commented Oct 3, 2023

Looks good, thanks @tyler-akamai!

Not to suggest increasing the scope of this work, but is there a technical reason why this is the only empty landing page that doesn't use the <ResourcesSection /> component?

Yeah I started to look into it but found that I needed to collect the different links from the documentation and ui team, and it seemed outside of the scope for this ticket. But I can create a new ticket to migrate the NodeBalancer empty state to <ResourceSection />

@jdamore-linode
Copy link
Contributor

Yeah I started to look into it but found that I needed to collect the different links from the documentation and ui team, and it seemed outside of the scope for this ticket. But I can create a new ticket to migrate the NodeBalancer empty state to <ResourceSection />

Makes sense! A separate ticket sounds good to me. Thanks again @tyler-akamai!

@mjac0bs
Copy link
Contributor

mjac0bs commented Oct 3, 2023

is there a technical reason why this is the only empty landing page that doesn't use the <ResourcesSection /> component?

@jdamore-linode I think this is because the <ResourcesSection/> component was added back in May, amidst a series of updates to landing pages that didn't already have resource links on them. (List of PRs here). NodeBalancer landing links look like they were last updated 3 years ago, so they already existed at that time and therefore the empty state landing page probably wasn't considered as part of those updates.

++ to @tyler-akamai for a separate ticket.

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

LGTM! confirming NodeBalancer top spacing is consistent with other landing pages Volumes, kubernetes and Domains

@bnussman-akamai
Copy link
Member

bnussman-akamai commented Oct 3, 2023

Looks better but I wonder if it makes sense to go ahead and refactor this to use <ResourcesSection /> like all of our other Empty states?

Edit: Oops, just read the other comments! Carry on 🫣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants