Skip to content
This repository was archived by the owner on Jan 16, 2024. It is now read-only.

Patch CodeModal and TitleCardList #266

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Patch CodeModal and TitleCardList #266

merged 3 commits into from
Jan 12, 2024

Conversation

wackerow
Copy link
Member

@wackerow wackerow commented Jan 6, 2024

Description

  • Overrides ModalCloseButton styling to force direction-responsive absolute positioning using insetInlineEnd instead of default right... fixes overlapping text on RTL locales
  • Patches use of ml with ms for code example list positioning on desktop
  • Fixes text color on hover for code example items using dark mode
  • Refactors CodeModal using latest code conventions

Related Issue

Close button on code example modal (homepage) was overlapping with label for RTL langs:
image

Contrast ratio was un-readable when hovering code example options in dark mode, and right margin incorrect:
image

With fixes:
image

image image

use direction-responsive margin, apply black hover text color to all child text (fixes contrast ratio on dark mode hovering)
Copy link

netlify bot commented Jan 6, 2024

Deploy Preview for ethereum-org-fork ready!

Name Link
🔨 Latest commit 3125728
🔍 Latest deploy log https://app.netlify.com/sites/ethereum-org-fork/deploys/65989c0f6aec630008653ac6
😎 Deploy Preview https://deploy-preview-266--ethereum-org-fork.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

Nice!

Left one comment about using style props if possible.

Comment on lines +29 to +32
style={{
right: "unset",
insetInlineEnd: "var(--eth-sizes-4)",
}}
Copy link
Member

Choose a reason for hiding this comment

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

This is a chakra component. Why not use style props?

Suggested change
style={{
right: "unset",
insetInlineEnd: "var(--eth-sizes-4)",
}}
right="unset"
insetInlineEnd="4"

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried this to start but I'm realizing that the insetInlineEnd style prop does not seem to be properly recognized by our Chakra components... this has let to a handful of bugs throughout the site where we replaced "left" or "right" with these style props and are not being recognized

For example, https://staging--ethereumorg.netlify.app/ar/roadmap If you go to our current staging branch, this page is set to rtl but our use of these style props in the hero are not working, which is why the Title box is still on the left.

I have another PR open on the canonical repo to address some of these:
https://github.com/ethereum/ethereum-org-website/pull/11905/files

Not 100% sure why these specific style props don't seem to be recognized... @TylerAPfledderer Any idea on this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

We may be able to get away with right="unset", but the other I've found only works if I nest inside style.

And small alternative as well if we want to make it a little more Chakra-native, could use const insetInlineEnd = useToken("space", "4") and then style={{ insetInlineEnd }}

Copy link
Member

Choose a reason for hiding this comment

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

Ah okok, no worries then. Didn't know about that problem 🤔

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.

🚀

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

Successfully merging this pull request may close these issues.

2 participants