-
Notifications
You must be signed in to change notification settings - Fork 166
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 misleading icon import documentation #5100
Conversation
@@ -131,7 +131,14 @@ If you use a limited set of icons you may want to include them individually to r | |||
@include vf-p-icons-common; | |||
|
|||
// include only the icons that you use in your project | |||
{% for icon in standard_icons + status_icons + social_icons %}@include vf-p-icon-{{ icon }}; | |||
{% for icon in standard_icons + status_icons + social_icons if icon != 'chevron-down' -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in review before, would it be possible to move the logic from here somewhere to where lists of icons are created, and maybe create a separate list of icon_mixins
to iterate through here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised. Now I create a list of base_icon_mixin_names
(contains the standard, status, & social icons). I didn't combine them all into a single icon_mixins
list as the "additional icons" are visually separated from the rest in the import documentation and have a comment separately inserted, so I wanted to keep that in place.
c75ea61
to
7eaa897
Compare
rebase of fork commits
7eaa897
to
7a3b053
Compare
@jmuzina Would that need updating now that we have chevron-left and chevron-right merged? |
Yes, I've just pushed an update to make it compatible with new chevron icons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit pick on using Jinja comments instead of HTML ones.
fd0621b
to
3bc7e0e
Compare
Done
Fixes misleading documentation for importing icon mixins. The current documentation directs users to import 3 mixins that do not exist (and would cause build errors if used):
vf-p-icon-chevron-up
vf-p-icon-chevron-down
vf-p-icon-information
This updates the documentation to direct users to import
vf-p-icon-chevron
andvf-p-icon-info
instead.Fixes #5094, WD-11768
Problem
The icons page uses the same array of icon names to do two different things:
p-icon--
.vf-p-icon--
.The code is making the assumption that mixin names and icon class names are exactly the same except for the prefix
vf-
for mixins andp-
for icons, and that each icon has its own dedicated mixin that matches this naming convention. This is mostly true, but is incorrect in three cases:vf-p-icon-chevron-up
(mixin does not exist; classp-icon--chevron-up
is generated by mixinvf-p-icon-chevron
) (src)vf-p-icon-chevron-down
(mixin does not exist; classp-icon--chevron-down
is generated by mixinvf-p-icon-chevron
) (src)vf-p-icon-information
(mixin does not exist; classp-icon--information
is generated by mixinvf-p-icon-info
) (src)Solutions
Creating the missing mixins; deprecate the old ones (alternate / more permanent solution)
We could create mixins to match the import instructions. These would create duplicate CSS to the current mixins but would fix the incorrect documentation without editing the Jinja template. While this PR does not do this, we should probably change the mixin names at some point in the future (next major version perhaps) in the interest of consistency.
Altering the Jinja template to fix the imports (PR / temporary solution)
A much simpler fix that does not alter our SCSS would be to update the icons Jinja template to fix the import instructions. This makes the Jinja template a bit uglier but has no effect on our users' SCSS (besides pointing them towards the right mixins to import).
Note that my first approach for this solution was simply hard-coding the correct mixins. However this is a bit brittle for my taste so I added some logic to the standard imports loop in Jinja to solve the problem.
QA
vf-p-icon-chevron-up
,vf-p-icon-chevron-down
, andvf-p-icon-information
have been removed.vf-p-icon-chevron
andvf-p-icon-info
have been added.Check if PR is ready for release
If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots
Before:
After: