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

Make icons in alerts optional and use masks instead of FA font #403

Merged
merged 5 commits into from
Feb 28, 2023

Conversation

justinkruit
Copy link
Member

This is still a concept, but already works great. This is an idea I discussed with @crftwrk before to replace the use of the Font Awesome font directly and make use of SVGs. This is because some people use the JS version of Font Awesome (like myself) which doesn't include the fonts anymore. Unfortunately using SVGs directly isn't easy as you can't change the color unless you want to add a fill to the url of the svg (making it less dynamic). Using masks we can get the same result.

The -webkit is needed unfortunately, as it is supported by Chrome but needs the -webkit for it to read it. More info: https://caniuse.com/css-masks

@crftwrk
Copy link
Member

crftwrk commented Feb 17, 2023

Really cool 👍

@justinkruit justinkruit marked this pull request as ready for review February 28, 2023 17:09
@justinkruit
Copy link
Member Author

I did a bit more testing of this, and honestly can't find anything wrong with it.

@crftwrk If you're happy with it, I'm fine with it being merged.

@crftwrk
Copy link
Member

crftwrk commented Feb 28, 2023

I love it ;-)

But we need icons for primary, secondary, light and dark alert as well https://dev.bootscore.me/theme/cheatsheet/#alerts. But I never have seen a site which uses those alerts and I have no idea which icon can be used for them. I think we should hide them quick and dirty there:

.alert-primary,
.alert-secondary,
.alert-light,
.alert-dark {
  &::before {
    display: none;
  }
}

Agree?

@justinkruit
Copy link
Member Author

Is it also possible to only have the icons when loading WooCommerce alerts and/or with an additional class? I personally think that that having the icons at all times feels like overwriting the functionality of the Bootstrap alerts.

@crftwrk
Copy link
Member

crftwrk commented Feb 28, 2023

I personally think that that having the icons at all times feels like overwriting the functionality of the Bootstrap alerts.

You're right. Let's do them only in Woo. In this case we can delete _alerts.scss and add code here https://github.com/bootscore/bootscore/blob/main/scss/bootscore_woocommerce/_wc_alerts.scss

@justinkruit
Copy link
Member Author

Already working on an alternative, ready in a few minutes 😉

@crftwrk
Copy link
Member

crftwrk commented Feb 28, 2023

Perfect ;-)

While we are here, we can replace @extend .mt-3; by margin-top: $spacer; in line 33. Seems better to me.

@justinkruit
Copy link
Member Author

The changes in a471254 will make the icon in the alert an option for everyone using alerts in bootScore, and extendable to the WooCommerce alerts.

@justinkruit
Copy link
Member Author

justinkruit commented Feb 28, 2023

Now I'm just thinking... Would a471254 be the correct solution, or also having a class .alert-icon to set the before element and .alert-[type]-icon to set the icon itself? That way people could easily expend the icon usage however they want.

crftwrk
crftwrk previously approved these changes Feb 28, 2023
Copy link
Member

@crftwrk crftwrk left a comment

Choose a reason for hiding this comment

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

Looks good 👍

The .alert-[type]-icon is a nice solution. Has to be written down the docs.

@crftwrk crftwrk added the documentation Improvements or additions to documentation label Feb 28, 2023
@justinkruit
Copy link
Member Author

The ::before part of the alert is now only being applied when adding the class .alert-icon to an alert. This way any styling (like the padding on the left) is not determined on the type of alert you're using, making it easily expandable.

If users want any alert to have an icon, they can use .alert .alert-danger .alert-icon .alert-danger-icon as classes.

To add or change an icon, use the following:

.alert-[name/type]-icon::before {
  content: ' ';
  --alert-icon: url("[url encoded svg here]");
}

Copy link
Member

@crftwrk crftwrk left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@crftwrk crftwrk changed the title Use masks instead of FA font Make icons in alerts optional and use masks instead of FA font Feb 28, 2023
@crftwrk crftwrk merged commit 2f35364 into main Feb 28, 2023
@crftwrk crftwrk deleted the alert-icon-masks branch April 11, 2023 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants