-
-
Notifications
You must be signed in to change notification settings - Fork 6.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
Redesign extended information page #4322
Conversation
4c06bee
to
61c3928
Compare
background: darken($ui-base-color, 4%); | ||
padding: 40px 0; | ||
|
||
.panel { |
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.
mostly just a comment, but the heavily nested scss style is much harder to read and maintain then the BEM style we use in the rest of the app. it makes it harder to override the content on the instance-owner side as well.
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 how much you want to fix here, I'm not asking for you to rewrite the whole about.scss file. but moving some of the smaller comments to it would be nice.
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.
Well this is reusing classes that were already in here. I'm not adding anything new.
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.
okay
app/javascript/styles/about.scss
Outdated
font-size: 14px; | ||
line-height: 24px; | ||
font-weight: 500; | ||
color: lighten($ui-base-color, 26%); |
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 use this lighten(ui-base, 26%) a ton of places in this code and in the other about page code, it should be pulled out into a scss variable.
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.
Maybe, but that's a refactoring task.
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.
Right, but this PR is adding new uses of 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.
left a few code style comments. otherwise lgtm
0b47839
to
f3923e6
Compare
When contact information is not set:
When contact information is set:
Mobile:
Textual changes: