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

Core 6.0 causes big icons #138

Closed
3 of 7 tasks
rbirkgit opened this issue May 27, 2022 · 6 comments · Fixed by #167, #168 or vmware-clarity/core#105
Closed
3 of 7 tasks

Core 6.0 causes big icons #138

rbirkgit opened this issue May 27, 2022 · 6 comments · Fixed by #167, #168 or vmware-clarity/core#105
Assignees
@ashleyryan
Labels
released on @12.x released type:fix workaround Workaround provided

Comments

@rbirkgit
Copy link

rbirkgit commented May 27, 2022 •
edited by ashleyryan

Describe the bug

Using Clarity Core 6.0 causes icons in menus, buttons etc to be 20px instead of 16px

How to reproduce

Use Core 6.0 (instead of 5.x) and not using Core style sheets.

  1. View min-height of icons in collapsed navigation

(v5) https://stackblitz.com/edit/clarity-v13-core-v5-icons
(v6) https://stackblitz.com/edit/clarity-v13-core-v6-icon

Expected behavior

Icons remain 16px with Core 6.0

Versions

Clarity version:

  • v12.x
  • v13.x
  • Other:

Framework:

  • Angular
  • React
  • Vue
  • Other:

Framework version:
Angular 13

Additional notes

Including Core styles fixes the issue, but Angular apps should not need to include them. In the mean time we have downgraded to Core 5.7.2 in all our apps.
@ashleyryan ashleyryan added type:fix workaround Workaround provided labels May 27, 2022
@rbirkgit
Copy link
Author

Seems the icon default size changed from:
var(--cds-global-space-7,calc((16 / var(--cds-global-base,20)) * 1rem))
to:
var(--cds-global-space-7,1rem)

ashleyryan pushed a commit to ashleyryan/ng-clarity that referenced this issue Jun 10, 2022
fix(vertical-nav): set min-height of nav icon
db7135b 
dont fall back to Core's min-height value when not using css tokens

closes vmware-clarity#138
@ashleyryan ashleyryan mentioned this issue Jun 10, 2022
Merged
13 tasks
ashleyryan pushed a commit to ashleyryan/ng-clarity that referenced this issue Jun 10, 2022
fix(vertical-nav): set min-height of nav icon
d799b14 
dont fall back to Core's min-height value when not using css tokens

closes vmware-clarity#138
ashleyryan pushed a commit to ashleyryan/ng-clarity that referenced this issue Jun 13, 2022
fix(vertical-nav): set min-height of nav icon
efd5931 
dont fall back to Core's min-height value when not using css tokens

closes vmware-clarity#138
@ashleyryan ashleyryan mentioned this issue Jun 13, 2022
Merged
13 tasks
ashleyryan pushed a commit that referenced this issue Jun 13, 2022
@ashleyryan
fix(vertical-nav): set min-height of nav icon
8360f52 
dont fall back to Core's min-height value when not using css tokens

closes #138
@rbirkgit
Copy link
Author

rbirkgit commented Jun 13, 2022 •
edited

Just want to make sure this fixes buttons / button bars too and not just the vertical menu. Asking as the fix only changes the vertical menu scss.

@ashleyryan
Copy link

I'm looking into a fix in Core that I think will address this more broadly, but you're right that this only fixes the vertical nav. Let me look into button icons as well.

ashleyryan pushed a commit that referenced this issue Jun 13, 2022
@ashleyryan
fix(vertical-nav): set min-height of nav icon
3554b29 
dont fall back to Core's min-height value when not using css tokens

closes #138
@ashleyryan ashleyryan reopened this Jun 13, 2022
@ashleyryan ashleyryan self-assigned this Jun 14, 2022
@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 13.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 12.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ashleyryan pushed a commit to vmware-clarity/core that referenced this issue Jun 14, 2022
fix(spacing): use base 20 fallback spacing logic
d65c3c0 
use 20 instead of 16 - 20 is the default described in the docs
allow for 16 to be set using --cds-global-base, even if the global css doesn't load

fixes vmware-clarity/ng-clarity#138
@ashleyryan ashleyryan mentioned this issue Jun 14, 2022
Closed
14 tasks
ashleyryan pushed a commit to ashleyryan/core that referenced this issue Jun 15, 2022
fix(spacing): use base 20 fallback spacing logic
cf792c0 
use 20 instead of 16 - 20 is the default described in the docs
allow for 16 to be set using --cds-global-base, even if the global css doesn't load

fixes vmware-clarity/ng-clarity#138
@ashleyryan ashleyryan mentioned this issue Jun 15, 2022
Merged
14 tasks
ashleyryan pushed a commit to ashleyryan/core that referenced this issue Jun 16, 2022
fix(spacing): use base 20 fallback spacing logic
b29a19b 
use 20 instead of 16 - 20 is the default described in the docs
allow for 16 to be set using --cds-global-base, even if the global css doesn't load

fixes vmware-clarity/ng-clarity#138
ashleyryan pushed a commit to vmware-clarity/core that referenced this issue Jun 16, 2022
@ashleyryan
fix(spacing): use base 20 fallback spacing logic
ab20ada 
use 20 instead of 16 - 20 is the default described in the docs
allow for 16 to be set using --cds-global-base, even if the global css doesn't load

fixes vmware-clarity/ng-clarity#138
@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2022

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@ashleyryan ashleyryan

Labels
released on @12.x released type:fix workaround Workaround provided
Projects
None yet
Milestone
No milestone
2 participants
@rbirkgit @ashleyryan