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

Improve button UI with icon #317

Merged
merged 5 commits into from Jun 21, 2022
Merged

Conversation

mparvazi
Copy link
Member

@mparvazi mparvazi commented Jun 9, 2022

Fixes #241

btn-with-icon:
Vertical alignment for icons inside buttons (inline-flex)

The size of icon and gap will be changed based on button size (btn-sm, btn, btn-lg):

Screenshot from 2022-06-11 17-43-16

Screenshot from 2022-06-11 17-42-13

Variables

$default-button-icon-size: 1.5rem;
$default-button-icon-gap: 0.25rem;

$small-button-icon-size: 1.25rem;
$small-button-icon-gap: 0.125rem;

$large-button-icon-size: 1.875rem;
$large-button-icon-gap: 0.375rem;

SCSS

.btn-with-icon {
  display: inline-flex;
  gap: $default-button-icon-gap;
  align-items: center;
  justify-content: center;

  .material-icons {
    font-size: $default-button-icon-size;
  }

  &.btn-sm {
    gap: $small-button-icon-gap;

    .material-icons {
      font-size: $small-button-icon-size;
    }
  }

  &.btn-lg {
    gap: $large-button-icon-gap;

    .material-icons {
      font-size: $large-button-icon-size;
    }
  }
}

@mparvazi mparvazi marked this pull request as draft June 9, 2022 12:06
@mparvazi mparvazi marked this pull request as ready for review June 9, 2022 13:49
@SharakPL
Copy link
Contributor

SharakPL commented Jun 10, 2022

All of these paddings and rtl can be avoided if you simply use gap on the button 😉

@mparvazi mparvazi marked this pull request as draft June 10, 2022 17:41
@mparvazi mparvazi marked this pull request as ready for review June 11, 2022 10:52
Copy link
Contributor

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

I guess it looks fine, but didn't tested it yet so!

@NeOMakinG NeOMakinG merged commit 0221cc6 into PrestaShop:develop Jun 21, 2022
@NeOMakinG
Copy link
Contributor

@Hlavtox @SharakPL if you find something wrong with this one afterwhile, please open an issue or a PR :)

Looked fine and wants to clean a bit the current PR flow of this repository in order to avoid stacking too much stuff

@SharakPL
Copy link
Contributor

Looks good for me.

One thing though. I see some duplications throughout the theme (not specific to this PR):

.text-gray {
color: $gray-700;
}
.w-full {
width: 100%;
}
.h-full {
height: 100%;
}

Can I remove these and just use Bootstrap classes w-100 and h-100 instead? text-muted isn't really the same as text-gray but I'm guessing the purpose of it is the same, so it also could be used here.

.cursor-pointer {
cursor: pointer;
}

This also could be removed. Adding role="button" to the element does exactly the same.

@NeOMakinG
Copy link
Contributor

Looks good for me.

One thing though. I see some duplications throughout the theme (not specific to this PR):

.text-gray {
color: $gray-700;
}
.w-full {
width: 100%;
}
.h-full {
height: 100%;
}

Can I remove these and just use Bootstrap classes w-100 and h-100 instead? text-muted isn't really the same as text-gray but I'm guessing the purpose of it is the same, so it also could be used here.

.cursor-pointer {
cursor: pointer;
}

This also could be removed. Adding role="button" to the element does exactly the same.

Feel free, you're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Material Icons are not aligned vertically inside the buttons
3 participants