-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Align label and data on show page #1522
Align label and data on show page #1522
Conversation
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.
Great stuff! One question: it looks like you have gone for baseline alignment. To me it looks a bit strange, but maybe it's just me:
If we are going for baselines, perhaps the label should be just one pixel further up?
Alternatively: perhaps the alignment should be centered instead? At about... 0.25em perhaps. Not sure!
I wonder if @LkeMitchll could provide some insight?
@@ -15,6 +15,7 @@ $heading-line-height: 1.2 !default; | |||
$base-border-radius: 4px !default; | |||
$base-spacing: $base-line-height * 1em !default; | |||
$small-spacing: $base-spacing / 2 !default; | |||
$top-space: 0.39em !default; |
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 feel that top-space
it a bit generic a name. Should this be something else? I guess it'll depend on whether cell-label
needs to be altered (see my comment above).
OK @davi-tapajos-codeminer42, let's see if we can see this through! I think an opinion from a designer may take a bit to arrive, so let's see what we can fix for now. What do you think of my comment about baseline vs centre alignment? Also, I think the name |
I think that centre alignment will looks nice! |
Cool. Would you be able to make those changes, and I'll review again? Thank you! |
I think this is good enough for now. Thank you again @davi-tapajos-codeminer42 Fixes #1213 |
Issue: #1213