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

Refactor: Migrate StakingHierarchy to Chakra #8686

Merged

Conversation

TylerAPfledderer
Copy link
Contributor

Description

Migrate the StakingHierarchy component to Chakra.

Also includes updating the glyph icons to have currentColor as their fill values.

Related Issue

#8632

@gatsby-cloud
Copy link

gatsby-cloud bot commented Nov 21, 2022

✅ ethereum-org-website-dev deploy preview ready

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Looking good @TylerAPfledderer

Just a few things I see comparing it with the prod version:

  • small gap between sections
    image

  • eth icons are too small on small resolutions
    image

This is in prod:
image

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented Dec 1, 2022

@pettinarip yikes! We have browser inconsistencies here. I definitely see this in Chrome, but the styling works as expected in Firefox.

It also looks broken in prod on Chrome too, or maybe the other way around?? 🤔

Update: Applying 100% width to the wrapper for each eth glyph will fix the size inconsistency between browsers. They will be forced to the max width and in turn allow the vertical bars to connect between each glyph,

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes @TylerAPfledderer.

I wouldn't say this is a blocker but I still see a small gap between the line and the eth logos. Probably it is related to the VStack but I see you set it as gap={0} in the md breakpoint, so I'm not sure what is going on.

image


.gold {
Copy link
Member

Choose a reason for hiding this comment

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

We will need to keep this class style since the markdown is using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add it back in and add a comment above it stating that markdown files use this classname

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented Dec 2, 2022

Thanks for the fixes @TylerAPfledderer.

I wouldn't say this is a blocker but I still see a small gap between the line and the eth logos. Probably it is related to the VStack but I see you set it as gap={0} in the md breakpoint, so I'm not sure what is going on.

image

With VStack I am supposed to use the spacing prop and not the gap prop (renders an "owl"-like selector to create margins). So even with gap margin still existed. 🤦🏼

For context and example, if spacing={2}, then the following selector and property gets rendered for every sibling after the first:

.css-tnhm38 > :not(style) ~ :not(style) {
  margin-top: 0.5rem;
}

gap would only really work if you were using the Flex component, say, or any component that can take this prop and doesn't do the above selector rendering under the hood.

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip should be good to go now! 😄

@pettinarip
Copy link
Member

Oh got it. makes sense now :)

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

lgtm!

@pettinarip pettinarip merged commit 8f06f27 into ethereum:dev Dec 5, 2022
@TylerAPfledderer TylerAPfledderer deleted the refactor/staking-heirarchy-chakra branch December 5, 2022 22:04
@corwintines corwintines mentioned this pull request Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants