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: refactor Info Label #1253

Merged
merged 2 commits into from
Jul 15, 2020
Merged

feat: refactor Info Label #1253

merged 2 commits into from
Jul 15, 2020

Conversation

InnaAtanasova
Copy link
Contributor

@InnaAtanasova InnaAtanasova commented Jul 14, 2020

Related Issue

Part of #1238

Description

This PR fixes one of the issues reported during Defect Hunting regarding Info Label with Icon missing border.

  • SCSS variables that were used only once in the code have been removed
  • The fd-info-label--only-icon class has been removed.

Screenshots

Before:

Screen Shot 2020-07-14 at 4 52 19 PM

After:

Screen Shot 2020-07-14 at 4 51 31 PM

Please check whether the PR fulfills the following requirements

@netlify
Copy link

netlify bot commented Jul 14, 2020

Deploy preview for fundamental-styles ready!

Built with commit a3120ad

https://deploy-preview-1253--fundamental-styles.netlify.app

@salarenko salarenko mentioned this pull request Jul 14, 2020
8 tasks
Copy link
Contributor

@salarenko salarenko left a comment

Choose a reason for hiding this comment

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

  • There is a difference in font size depending whether the icon has been used
    image

  • Why did you removed --icon-only class? I remember Info Label had to use some specific padding if used only with an icon.

src/mixins/_mixins.scss Show resolved Hide resolved
src/mixins/_mixins.scss Show resolved Hide resolved
@salarenko salarenko mentioned this pull request Jul 15, 2020
1 task
@InnaAtanasova
Copy link
Contributor Author

InnaAtanasova commented Jul 15, 2020

  • Why did you removed --icon-only class? I remember Info Label had to use some specific padding if used only with an icon.

The "default" padding is padding: 0 0.625rem;
These are the paddings when you have Text + Icon and Icon Only:

Screen Shot 2020-07-15 at 9 05 15 AM

It's clear that in both cases they are the same. I don't see the need of having an additional class doing nothing :)
Screen Shot 2020-07-15 at 9 07 17 AM

@InnaAtanasova
Copy link
Contributor Author

  • There is a difference in font size depending whether the icon has been used
    image

@salarenko I fixed this

@droshev droshev self-requested a review July 15, 2020 18:53
@InnaAtanasova InnaAtanasova merged commit a44f0b5 into master Jul 15, 2020
@InnaAtanasova InnaAtanasova deleted the feat/info-label-refactoring branch July 15, 2020 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants