-
Notifications
You must be signed in to change notification settings - Fork 239
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
feat(insights): responsive layouts and components #2285
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
@@ -0,0 +1 @@ | |||
DISABLE_ACCESSIBILITY_REPORTING=true |
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.
Please don't check this config in; this is a local setting and isn't intended to be checked in. We can gitignore this file instead.
line-height: 125%; | ||
letter-spacing: letter-spacing("tight"); | ||
margin: 0; | ||
} |
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.
I found that I really didn't like these mixins much and I haven't been using them much; instead I started using the text
mixin a few lines below, e.g. this would be:
.someClass {
@include theme.text("base", "medium");
line-height: 125%;
letter-spacing: theme.letter-spacing("tight");
}
I suppose the line heights & letter spacing could get redundant, but I found there to be so much variations in the different combinations of text properties that it didn't seem to make a lot of sense to try to abstract them all.
I'm open to other ideas here though, this is one place I don't feel great about the theming system right now...
} | ||
|
||
.mobileMenuOverlay { | ||
background: rgba(0,0,0,0.4); |
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.
rgba
is effectively deprecated and should be replaced with rgb
instead: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb
So this would be:
background: rgb(0 0 0 / 40%);
or (I prefer this):
background: rgb(from black r g b /40%)
background: rgba(0,0,0,0.4); | ||
position: fixed; | ||
inset: 0; | ||
z-index: 999; |
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.
hmm I generally recommend against this kind of thing, using super high z-indexing generally shouldn't be necessary and if it is, it means something else is broken with stacking contexts and it will cause hard to debug issues elsewhere.
I suggest avoiding z-indexing if possible, but if not possible just set it to the minimum necessary value. You can usually avoid z-indexing issues simply by setting stacking contexts correctly and explicitly (sometimes you need to use isolate
to create a stacking context if you aren't implicitly creating one where one is needed.
If you see issues with stacking context elsewhere when using the minimum necessary z-indexing, it's best to try to resolve those issues early to avoid causing hard to debug problems down the line.
In this particular case, however, I think you can get away without having to use this entirely if you instead use the ModalDialog
component as I mention below -- that component will render to a portal at the document root that is properly overlaid and it will avoid all issues with z-indexing entirely.
bottom: 0; | ||
left: 0; | ||
right: 0; | ||
padding: 1rem; |
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.
Let's use theme.spacing(4)
instead
.mobileMenuHandle { | ||
background: theme.color("background", "secondary"); | ||
width: 33%; | ||
height: 6px; |
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.
We should avoid px values entirely and stick to theme.spacing
flex-flow: row nowrap; | ||
align-items: center; | ||
gap: theme.spacing(3); | ||
text-transform: capitalize; |
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.
We shouldn't use text-transform
, I know we aren't localizing but if we ever do, text-transforms aren't consistently accurate across locales. For instance, in some languages, it is incorrect to have some characters capitalized in some contexts even if the rest of the text is all-caps. It's also a completely unnecessary added amount of processing for the browser.
Instead, we should just write the string as it's meant to be rendered. If you want all caps, just put all caps in the html. If we ever go to localize, this will save us a ton of headache, and it will reduce unnecessary processing in the meantime.
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.
This one is to Capitalize the theme
return (it returns light, dark, system
), otherwise I wouldn't use it
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.
It's still best to map that in the jsx side, not the css
7291e47
to
3ac2c47
Compare
…ts rendering in PriceFeeds
3ac2c47
to
b952f18
Compare
…veness and layout adjustments in component library
…eness across breakpoints in the insights component
…for improved user experience and layout in insights section
… for improved user experience across mobile and desktop breakpoints
…ileMenu, Navigation, and PriceFeeds for improved consistency and layout
…with improved title and icon integration
… across various components for consistent branding and usability
…text responsiveness and remove unnecessary outline in Publishers
…Layout for consistent structure and remove deprecated PageTitle component
Chores around making Insights Hub layout and components responsive.