-
Notifications
You must be signed in to change notification settings - Fork 152
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: improve result card reflow breakpoints #3372
Conversation
// are presented side-by-side or wrapped | ||
padding-top: 2px; | ||
padding-bottom: 14px; | ||
padding-left: 0px; |
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 tried removing this line and it had no effect on the styling. We might be able to get rid of this.
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 agree that it isn't currently necessary; I included it (and the 0 margins a few lines down) because I found it more readable to be explicit (there are some elements that browsers will use a 1px padding/margin for by default, and this saves a reader/reviewer from having to think about that/look up whether it applies)
margin-top: 12px; | ||
margin-bottom: 0px; | ||
margin-left: 20px; | ||
margin-right: 0px; |
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 tried removing this line as well and it also had no effect on the styling.
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.
see above
} | ||
|
||
.instance-list-row-content { | ||
text-align: right; | ||
border-bottom: 0.5px solid $neutral-10; |
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.
nit: we can also write this like so:
+ .row {
border-top: 0.5px solid $neutral-10;
}
Adjacent elements can have styles applied to them like ($selector + $selector) to apply styles to all but the first element (hence why border-top over border-bottom). Yes, the nittiest of nits but I always like sharing this cool selector usage. Link to some text on it: https://alistapart.com/article/axiomatic-css-and-lobotomized-owls/
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 agree that this would work equally well, but I didn't use it because I think it's significantly harder for a reader to reason about, despite being shorter:
- if someone don't know the advanced selectors off the top of their head, "last-child" is a lot easier to infer the meaning of than "+"
- even if I do remember that "+" is an adjacent sibling selector, I have to remember whether it refers to the "left" or "right" sibling to understand if the css should be using -top or -bottom in a code review
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.
Minor suggestions, otherwise LGTM
Description of changes
Previously, the reflow behavior for cards (switching from "row headers to the left of content" to "row headers above content" at small widths) was driven using a media query over screen width. This worked OK in web and in reports, where cards are given the entire width of the screen to work with, but caused poor behavior in unified, where the cards are competing for screen width with the screenshot panel. In the most pathological width, you can see something like this:
Before:
This PR addresses this by redoing the reflow behavior for cards in terms of
display:flex
on the card rows, so the reflow behavior will be based on the width of the cards, not the width of the screen.Per discussion with @ferBonnin, as part of this update, I've fixed up a few padding/margin sizes that were off from Figma's suggestions (in particular, we were using a width for the labels on android that was way larger than spec). With these changes, it was possible to combine the web/unified behavior and remove the unified-specific override by accepting web's padding being 4px higher than figma, so we did that (the result is still closer to the figma than the "before" case).
I've reviewed each of these cases and encourage reviewer(s) to build the code locally and do the same:
Unified Automated Checks (100% zoom):
Before:
After:
Unified Report (150% zoom):
Before:
After:
Web FastPass Automated Checks (125% zoom):
Before:
After:
Web FastPass Needs Review (120% zoom):
Before:
After:
Pull request checklist
yarn fastpass
yarn test
)<rootDir>/test-results/unit/coverage
fix:
,chore:
,feat(feature-name):
,refactor:
). SeeCONTRIBUTING.md
.