-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report: center and style top-level runtimeError #9271
Conversation
also, can this fix up for #7011? |
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.
LGTM
margin-left: auto; | ||
margin-right: auto; | ||
width: 50%; | ||
background-color: var(--color-amber); |
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.
feels like we might want a bit more padding with the new background style 🤷♂
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.
done and updated images
@exterkamp since this is his thing |
@@ -35,6 +35,7 @@ | |||
.lh-vars { | |||
/* Palette using Material Design Colors | |||
* https://www.materialui.co/colors */ | |||
--color-amber: #FFF8E1; /* 50 */ |
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.
not sure what 50 means
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.
yeah I was confused too until I looked up the material ui table :) https://www.materialui.co/colors
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.
oh, in that case, should be part of the name.
Omit "-{intensity}" if there is only one color of that hue.
isn't simplifying anything if it's helpful enough to have the intensity that we add it in a comment :)
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.
done
padding: var(--category-padding); | ||
margin-left: auto; | ||
margin-right: auto; | ||
width: 50%; |
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.
50% seems a little arbitrary and has the box edges not matching up with the "grid" of the report.
max-width: calc(var(--report-width) - var(--category-padding) * 2);
this'll have it match most of the report content.
} | ||
@media screen and (max-width: 964px) { |
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.
once the max-width is applied you can drop this whole MQ and rule
Fixes #7011
Before:
See #9270
After: