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.

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

feat(sbb-header): introduce size s #3047

Merged
merged 23 commits into from
Sep 12, 2024
Merged

Conversation

DavideMininni-Fincons
Copy link
Contributor

@DavideMininni-Fincons DavideMininni-Fincons commented Aug 30, 2024

Draft - to display the logo without text a new property has been added to the sbb-logo component.

If the API change is fine, the final visual result should be validated by @mcilurzo, otherwise a different solution (new SVG? new component?) should be find.

EDIT:

  • API change is fine (signetOnly is the property proposed name)
  • no need of a size prop on the header-button/link

@DavideMininni-Fincons DavideMininni-Fincons linked an issue Aug 30, 2024 that may be closed by this pull request
@github-actions github-actions bot temporarily deployed to pr3047 August 30, 2024 07:53 Inactive
@github-actions github-actions bot temporarily deployed to pr3047-diff August 30, 2024 07:54 Inactive
src/elements/header/header/readme.md Outdated Show resolved Hide resolved
src/elements/core/styles/core.scss Show resolved Hide resolved
src/elements/header/header/header.scss Show resolved Hide resolved
src/elements/logo/logo.scss Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pr3047 September 2, 2024 13:12 Inactive
@github-actions github-actions bot temporarily deployed to pr3047-diff September 2, 2024 13:13 Inactive
@mcilurzo mcilurzo added the pr: ux review required A ux review is required for this pull request label Sep 4, 2024
@DavideMininni-Fincons
Copy link
Contributor Author

ux review: https://www.figma.com/design/9r6xSfNmEfCFxl1yFYedrj?node-id=43777-147301#935958908 https://www.figma.com/design/9r6xSfNmEfCFxl1yFYedrj?node-id=43777-147442#935950489 https://www.figma.com/design/9r6xSfNmEfCFxl1yFYedrj?node-id=43910-264561#936057771

@mcilurzo
I added the application name/version in stories size=s, applying some base styling class
may I ask to you to recheck the height? i see it correctly

16px
Screenshot 2024-09-04 alle 16 41 08

20px from medium
Screenshot 2024-09-04 alle 16 41 44

@github-actions github-actions bot temporarily deployed to pr3047 September 5, 2024 09:32 Inactive
@github-actions github-actions bot temporarily deployed to pr3047-diff September 5, 2024 09:33 Inactive
@github-actions github-actions bot temporarily deployed to pr3047 September 12, 2024 13:09 Inactive
@github-actions github-actions bot temporarily deployed to pr3047-diff September 12, 2024 13:09 Inactive
@github-actions github-actions bot temporarily deployed to pr3047 September 12, 2024 13:57 Inactive
@github-actions github-actions bot temporarily deployed to pr3047-diff September 12, 2024 13:57 Inactive
Copy link
Contributor

@jeripeierSBB jeripeierSBB left a comment

Choose a reason for hiding this comment

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

LGTM, in case #3080 is merged first, in visual tests we could use stateElement for focus tests instead of custom tabkey solution

@mcilurzo mcilurzo added pr: ux review approved Pull request has been approved by a ux review and removed pr: ux review required A ux review is required for this pull request labels Sep 12, 2024
@mcilurzo
Copy link
Contributor

LGTM. Thank you!

@DavideMininni-Fincons DavideMininni-Fincons merged commit cd60922 into main Sep 12, 2024
23 of 24 checks passed
@DavideMininni-Fincons DavideMininni-Fincons deleted the feat/sbb-header-size-s branch September 12, 2024 15:51
@github-actions github-actions bot added pr: peer review required A peer review is required for this pull request and removed pr: peer review required A peer review is required for this pull request labels Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diff-available pr: lead review approved Pull request has been approved by a lead review pr: peer review required A peer review is required for this pull request pr: ux review approved Pull request has been approved by a ux review pr: visual review required preview-available
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header: Size S
3 participants