Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(button): update button sizes #103

Merged
merged 30 commits into from
Jul 22, 2022
Merged

fix(button): update button sizes #103

merged 30 commits into from
Jul 22, 2022

Conversation

gfellerph
Copy link
Member

This change updates button dimensions according to the design and reduces the file size of the button.css to ~7kb uncompressed because the arrow icon is now made with CSS and can make use of currentColor instead of a background icon which had to be "colored" by inversion.

@gfellerph
fix(button): update static dimensions
a01aed1
@gfellerph
fix(buttons): update animation distance for animated buttons
72231d8
@gfellerph
refactor(button): use custom built arrow
6d9d699 
This change reduces the file size of the button.css to ~7kb uncompressed because the arrow icon is now made with CSS and can make use of currentColor instead of a background icon which had to be "colored" by inversion.
@gfellerph gfellerph linked an issue May 23, 2022 that may be closed by this pull request
Update button sizes #40
Closed
@changeset-bot
Copy link

changeset-bot bot commented May 23, 2022 •
edited

🦋 Changeset detected

Latest commit: 0cde4d3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@swisspost/web-demo Patch
@swisspost/web-styles Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

🚀 Preview environment ready: https://preview-103--swisspost-web-frontend.netlify.app/

@gfellerph gfellerph requested review from a team, Janobob and alizedebray May 23, 2022 12:16
@gfellerph
Copy link
Member Author

@swisspost/design https://preview-103--swisspost-web-frontend.netlify.app/#/bootstrap-samples/buttons

@moniesan moniesan self-requested a review May 24, 2022 07:58
wueestd
wueestd suggested changes May 24, 2022
View reviewed changes
Copy link

@wueestd wueestd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deftaul State

@gfellerph
Copy link
Member Author

gfellerph commented Jun 9, 2022 •
edited

  • Safari shows blue text on some button variants
  • Primary buttons on dark backgrounds should fade from white to 0.8 white on hover, not the other way round

@gfellerph gfellerph requested a review from wueestd June 14, 2022 12:46
@github-actions
Copy link
Contributor

🚀 Preview environment ready: https://preview-103--swisspost-web-frontend.netlify.app/

@gfellerph gfellerph requested a review from Majis14 June 16, 2022 11:30
@gfellerph
Copy link
Member Author

@swisspost/design, this PR can be reviewed again at https://preview-103--swisspost-web-frontend.netlify.app/#/bootstrap-samples/buttons. Thx for reviewing.

wueestd
wueestd suggested changes Jun 21, 2022
View reviewed changes
Copy link

@wueestd wueestd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed I would change the following:
negativ button: only change the button border to grey, let the text on white

@gfellerph gfellerph mentioned this pull request Jun 27, 2022
Closed
alizedebray
alizedebray requested changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@alizedebray alizedebray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Our buttons look first-rate now!

I simply noticed these few differences with the specified design:
.btn-sm
- border-radius: 3px

.btn-default
- padding-right & padding-left: 2px narrower for all sizes (because of the side borders)
- color: 0.6 opacity on light backgrounds & on hover on dark backgrounds

.btn-default[disabled]
- color: 0.7 opacity on light backgrounds & 0.2 opacity on dark backgrounds
- border-color: 0.2 opacity on light backgrounds & 0.7 opacity on dark backgrounds
To get expected result, the buttons should also have 0.4 opacity

.btn-link
- padding-right & padding-left: 6px for all sizes
- color: 0.6 opacity on light backgrounds & on hover on dark backgrounds

.btn-link.btn-animated
- ::after: should probably not show on hover

...bootstrap/components/buttons/buttons-demo/icon-buttons-text/icon-buttons-text.component.html Outdated Show resolved Hide resolved
packages/web-styles/src/components/button.scss Outdated Show resolved Hide resolved
@gfellerph
docs(icons): adds aria-hidden for all icons on buttons
3b90669
@gfellerph
feat(buttons)!: remove size specific border radius variables
2ad1433 
All buttons have the same border radius.
@gfellerph
fix(buttons): prevents link buttons from showing animated icons
a97d1e2
@gfellerph
fix(buttons): uses static padding for link buttons
f28e826
@gfellerph
fix(buttons): remove color inherit on link buttons
9b8a1d5 
The color of buttons is defined in the .btn base class and reacts to light/dark backgrounds. The link button should be consistent with other buttons in this behaviour and not inherit the color.
@gfellerph
Copy link
Member Author

@alizedebray, thanks for the review! The color of hovered buttons should stay opaque for accessibility (#103 (review)) and the disabled button styles were improved to be as contrastful as possible (primary on light bg is not accessible).

oliverschuerch
oliverschuerch requested changes Jul 14, 2022
View reviewed changes
...app/bootstrap/components/buttons/buttons-demo/button-disabled/button-disabled.component.html Outdated Show resolved Hide resolved
...app/bootstrap/components/buttons/buttons-demo/button-disabled/button-disabled.component.html Outdated Show resolved Hide resolved
packages/web-demo/src/app/bootstrap/components/buttons/buttons-demo/buttons-demo.component.html Outdated Show resolved Hide resolved
packages/web-demo/src/app/bootstrap/components/buttons/buttons-demo/buttons-demo.component.html Outdated Show resolved Hide resolved
packages/web-demo/src/app/bootstrap/components/buttons/buttons-demo/buttons-demo.component.html Show resolved Hide resolved
packages/web-styles/src/components/button.scss Outdated Show resolved Hide resolved
packages/web-styles/src/components/button.scss Outdated Show resolved Hide resolved
packages/web-styles/src/components/timepicker.scss Outdated Show resolved Hide resolved
packages/web-styles/src/mixins/_button.scss Outdated Show resolved Hide resolved
packages/web-styles/src/placeholders/_button.scss Outdated Show resolved Hide resolved
oliverschuerch
oliverschuerch previously approved these changes Jul 22, 2022
View reviewed changes
@oliverschuerch oliverschuerch dismissed their stale review July 22, 2022 08:24

Mistakenly approved

@sonarcloud
Copy link

sonarcloud bot commented Jul 22, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.5% 0.5% Duplication

@gfellerph gfellerph merged commit e280aae into main Jul 22, 2022
@gfellerph gfellerph deleted the 40-update-button-sizes branch July 22, 2022 12:58
@github-actions github-actions bot mentioned this pull request Jul 22, 2022
Merged
@github-actions github-actions bot mentioned this pull request Nov 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverschuerch oliverschuerch oliverschuerch approved these changes

@swisspost-bot swisspost-bot swisspost-bot approved these changes

@Janobob Janobob Awaiting requested review from Janobob

@moniesan moniesan Awaiting requested review from moniesan

@Majis14 Majis14 Awaiting requested review from Majis14

@wueestd wueestd Awaiting requested review from wueestd

@alizedebray alizedebray Awaiting requested review from alizedebray

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update button sizes
6 participants
@gfellerph @oliverschuerch @wueestd @alizedebray @swisspost-bot @moniesan