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

fix: outline color of breakcrumb icon overflow #1969

Merged
merged 2 commits into from
Dec 18, 2020

Conversation

stefanoScalzo
Copy link
Contributor

@stefanoScalzo stefanoScalzo commented Dec 14, 2020

Related Issue

Closes #1892

Description

Make all states have the same color for both items

Screenshots

Before:

image

After:

Screen Shot 2020-12-14 at 1 05 46 PM

Please check whether the PR fulfills the following requirements

  1. The output matches the design specs
  • All values are in rem
  • Text elements follow the truncation rules
  • hover state of the element follow design spec
  • focus state of the element follow design spec
  • active state of the element follow design spec
  • selected state of the element follow design spec
  • selected hover state of the element follow design spec
  • pressed state of the element follow design spec
  • Responsiveness rules - the component has modifier classes for all breakpoints
  • Includes Compact/Cosy/Tablet design
  • RTL support
  1. The code follows fundamental-styles code standards and style
  • only one top level fd-* class is used in the file
  • BEM naming convention is used
  • Mixins are used for repeatable code (fd-rtl, fd-ellipsis, fd-flex, fd-selected, fd-focus, ect.)
  • A11y support - keyboard support, screenreader support, proper ARIA attributes, etc.
  • fd-reset() mixin is applied to all elements
  • Variables are used, if some value is used more than twice.
  • Checked if current components can be reused, instead of having new code.
  1. Testing
  • tested Storybook examples with "CSS Resources" normalize option
  • tested Storybook examples with "CSS Resources" unnormalize option
  • Verified all styles in IE11
  • Updated tests
  1. Documentation
  • Storybook documentation has been created/updated
  • Breaking Changes wiki has been updated in case of breaking changes.

@stefanoScalzo stefanoScalzo added this to the Sprint 52 - Hilo milestone Dec 14, 2020
@stefanoScalzo stefanoScalzo requested a review from a team December 14, 2020 18:10
@stefanoScalzo stefanoScalzo self-assigned this Dec 14, 2020
@netlify
Copy link

netlify bot commented Dec 14, 2020

✔️ Deploy preview for fundamental-styles ready!

🔨 Explore the source changes: 7956807

🔍 Inspect the deploy logs: https://app.netlify.com/sites/fundamental-styles/deploys/5fda14ebd6d2d500075183b9

😎 Browse the preview: https://deploy-preview-1969--fundamental-styles.netlify.app

Comment on lines 59 to 78
.#{$block}__dropdown-icon {
color: var(--sapLink_Active_Color, #0a6ed1);
}
}

&:active {
.#{$block}__dropdown-icon {
color: var(--sapLink_Active_Color, #0a6ed1);
}

&:hover {
.#{$block}__dropdown-icon {
color: var(--sapLink_Active_Color, #0a6ed1);
}
}
}

&:hover {
.#{$block}__dropdown-icon {
color: var(--sapLink_Hover_Color, #0854a0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you adding the colors this way?
Should be just color: var(--sapLink_Hover_Color); or `color: var(--sapLink_Active_Color)

Copy link
Contributor

Choose a reason for hiding this comment

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

and since var(--sapLink_Active_Color) is repeated 3 times maybe put it in a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed we have fallbacks in some components but ill change it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not the proper way to add a fallback in general. You will need:

color:  #0854a0;
color: var(--sapLink_Hover_Color, #0854a0);

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I searched the project and couldn't find such case :)
There's one usage in custom.scss file which is for the documentation. I think you are talking about fundamental-ngx, but this is another case.

@JKMarkowski JKMarkowski changed the base branch from master to main December 17, 2020 10:53
@stefanoScalzo stefanoScalzo merged commit b1d6f50 into main Dec 18, 2020
@stefanoScalzo stefanoScalzo deleted the fix-breadcrumb-hover branch December 18, 2020 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

outline color different on breadcrumb hover
3 participants