-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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(Trilemma): update Chakra implementations #10490
refactor(Trilemma): update Chakra implementations #10490
Conversation
✅ ethereum-org-website-dev deploy preview ready
|
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
9d5d0ae
to
6cfa88e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @TylerAPfledderer, solid work here... gave it a run through and looks good, just a couple notes. From the PR description didn't quite sound like layout changes were part of this, so just left a couple notes of where it looked off compared to production and how I think we can get it closer.
With those few adjustments I think we can get this in... I'll circle back on this and if you're too busy I'm happy to patch it up
src/components/Trilemma/index.tsx
Outdated
pl={{ lg: 12 }} | ||
pr={{ lg: 32 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're adding this here, we'll need to also remove the margins on <chakra.svg
inside Triangle.tsx
(L115-117).. we shouldn't need mt
since we're aligning center
, and we shouldn't need me
since we handle this here. I think we still need my
with how it's set up to keep the height within reason on mobile.
We should make these RTL-friendly though:
pl={{ lg: 12 }} | |
pr={{ lg: 32 }} | |
ps={{ lg: 12 }} | |
pe={{ lg: 32 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied RTL-aware props.
At the very least, top margin should be considered, as even with larger screens, there are certain points where the text wrapping expands there container height be too close to the bounds of the section container compared to how much spacing there is consistently through the breakpoints. center alignment primary helps the triangle svg though.
d1dacb9
to
6343bfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job @TylerAPfledderer! Approving this from a dev perspective but would like to have a designer approval before finally pulling this in. cc: @nloureiro @konopkja
Not positive we can use "2xl" for a borderTopRadius
but I'm fairly certain we can... if you agree, feel free to commit that suggestion, but not a blocker.
TODO:
- Designer approval
lg: "0 1 500px", | ||
}} | ||
> | ||
<OldHeading fontSize="2rem" mt={0}> | ||
<Heading fontSize="2rem"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nloureiro Brought this up elsewhere, but more use of a non-standard font size
^ These are the sizes we have assigned to tokens right now... not requesting anything for this PR, just noting... card here
update `borderTopRadius` value on `DrawerContent` usage Co-authored-by: Paul Wackerow <54227730+wackerow@users.noreply.github.com>
it's better with the spacing Co-authored-by: Paul Wackerow <54227730+wackerow@users.noreply.github.com>
Thanks again @TylerAPfledderer!! |
Description
Updates usage of Chakra UI for the
Trilemma
component.Flex
withStack
Stack
should be the preferred choice overFlex
if the defaultflexDirection
should becolumn
if there is no expectation in changing torow
at a breakpoint.forwardRef
to theCircleSelect
component for passing event props.children
type signature from svgText
component as it is already present inHTMLChakraProps
DrawerContent
and the childCard
hideBelow
andhideFrom
as the compiledlg
breakpoint values renders correctly.Stack
component wrapping the text content, remove the baseflex
prop value as it does not need to be declared for the breakpoint object syntax to compile correctly.Preview link
Related Issue
N/A