-
Notifications
You must be signed in to change notification settings - Fork 24
Core: declared font size using relative units #4003
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
base: develop
Are you sure you want to change the base?
Core: declared font size using relative units #4003
Conversation
…eclare-font-size-relative-units
} | ||
} | ||
|
||
:host(.md) { | ||
ion-badge { | ||
--padding-top: 3px; | ||
--padding-end: 5px; |
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.
Consider using design tokens. Consider resizing to match the design system’s 2, 4, 8 spacing scale.
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.
These values ensures that the badge design follows the design spec, and changing them would cause the badge to look differently.
I just removed some because they are no longer relevant, since ion-badge already uses that size for top/bottom 😊
--padding-end: 5px; | ||
--padding-bottom: 3px; | ||
--padding-start: 5px; |
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.
Consider using design tokens. Consider resizing to match the design system’s 2, 4, 8 spacing scale.
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.
These values ensures that the badge design follows the design spec, and changing them would cause the badge to look differently.
I just removed some because they are no longer relevant, since ion-badge already uses that size for top/bottom 😊
}); | ||
|
||
it('should be rendered with correct dimensions', () => { | ||
expect(ionBadge).toHaveComputedStyle({ | ||
'min-width': size('s'), | ||
'min-height': size('s'), | ||
'padding-bottom': '3px', |
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.
Consider resizing px values to match the design system’s 2, 4, 8 spacing scale.
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.
These values ensures that the badge design follows the design spec, and changing them would cause the badge to look differently.
I just removed some because they are no longer relevant, since ion-badge already uses that size for top/bottom 😊
@@ -3,7 +3,7 @@ | |||
|
|||
$form-field-input-font-family: var(--kirby-font-family); | |||
$form-field-input-line-height: 1.5; | |||
$form-field-input-padding: 1em; | |||
$form-field-input-padding: utils.size('s'); | |||
$form-field-label-height: 24px; |
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.
Consider using design token for px
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.
Are you thinking of $form-field-label-height
? If you do, it is a good idea, but I think it actually relates more to the line-height issue than the font size issue, since this variable is concerned with the total height of the label in form field.
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.
Yes, I was thinking of the $form-field-label-height.
@@ -3,7 +3,7 @@ | |||
|
|||
$form-field-input-font-family: var(--kirby-font-family); | |||
$form-field-input-line-height: 1.5; | |||
$form-field-input-padding: 1em; | |||
$form-field-input-padding: utils.size('s'); | |||
$form-field-label-height: 24px; |
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.
Yes, I was thinking of the $form-field-label-height.
Which issue does this PR close?
This PR closes #3578 closes #3577
What is the new behavior?
All font sizes used are now based on relative units, and tests have been updated accordingly.
Limitations
The readability of multi-line paragraphs is reduced when using a large font size in the browser, as line-height is not yet reworked. But it is usable as is, and thus that change is handled on its own when #3579 is solved.
Component tweaks
Most components are left as is, as we generally do not want to scale spacing etc. according to font size, as the design would be too spacious, which can reduce coherence and clarity.
Flag, Badge and Input have been changed because the visual appearance is largely defined by the text content, namely to keep their appearance/affordance when text is scaled.
Click images to see larger version.
Before
After
across font-sizes, but otherwise fit as much text content inside in
the least amount of space. In order to make text and icon badges have
similar size, the icon now scales with text as well. The badge then
becomes quite large when used with e.g. an avatar in item, but
I think it is the lesser of two evils, and also helps with
instead of the badge being more likely to be overlooked.
Before
After
compared to other comps. It looks decent on its own,
but once used in for example items with small values
it also loses a bit of its visual affordance by
becoming square-ish.
Before
After
size. Apart from allowing more content to fit on screen,
the visual appearance is also better preserved
Does this PR introduce a breaking change?
Checklist:
The following tasks should be carried out in sequence in order to follow the process of contributing correctly.
Reminders
Review
When the pull request has been approved it will be merged to
develop
by Team Kirby.