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 misleading icon import documentation #5100

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented May 24, 2024

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 and vf-p-icon-info instead.

Fixes #5094, WD-11768

Problem

The icons page uses the same array of icon names to do two different things:

  • Generate the icons sections that render the example icons to the page. These are rendered using a macro that uses the values from the array to create the class name for the icon by prepending p-icon--.
  • Generate the import instructions. Jinja loops over the elements in the icons array and recreates mixin names by prepending 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 and p- 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; class p-icon--chevron-up is generated by mixin vf-p-icon-chevron) (src)
  • vf-p-icon-chevron-down (mixin does not exist; class p-icon--chevron-down is generated by mixin vf-p-icon-chevron) (src)
  • vf-p-icon-information (mixin does not exist; class p-icon--information is generated by mixin vf-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

  • Open demo icons page
  • Verify that imports for vf-p-icon-chevron-up, vf-p-icon-chevron-down, and vf-p-icon-information have been removed.
  • Verify that imports for vf-p-icon-chevron and vf-p-icon-info have been added.
  • Copy-paste the imports block into an IDE with SCSS support in a project that has Vanilla installed. Verify that all mixins are importable and your project builds.

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:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix relesase (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

Before:
Screenshot 2024-05-24 at 4 28 02 PM

After:
Screenshot 2024-05-24 at 4 27 51 PM

@jmuzina jmuzina self-assigned this May 24, 2024
@webteam-app
Copy link

@jmuzina jmuzina marked this pull request as ready for review May 24, 2024 20:47
@@ -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' -%}
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bartaz

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.

@jmuzina jmuzina force-pushed the fix-icon-import-documentation branch from c75ea61 to 7eaa897 Compare May 30, 2024 15:03
@jmuzina jmuzina added Bug 🐛 and removed Documentation 📝 Documentation changes or updates labels May 30, 2024
@jmuzina jmuzina force-pushed the fix-icon-import-documentation branch from 7eaa897 to 7a3b053 Compare May 30, 2024 16:51
@bartaz
Copy link
Contributor

bartaz commented Jun 4, 2024

@jmuzina Would that need updating now that we have chevron-left and chevron-right merged?

@jmuzina
Copy link
Member Author

jmuzina commented Jun 4, 2024

@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

Copy link
Contributor

@bartaz bartaz left a 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.

templates/docs/patterns/icons/index.md Outdated Show resolved Hide resolved
templates/docs/patterns/icons/index.md Outdated Show resolved Hide resolved
templates/docs/patterns/icons/index.md Outdated Show resolved Hide resolved
templates/docs/patterns/icons/index.md Outdated Show resolved Hide resolved
templates/docs/patterns/icons/index.md Outdated Show resolved Hide resolved
templates/docs/patterns/icons/index.md Outdated Show resolved Hide resolved
@bartaz bartaz force-pushed the fix-icon-import-documentation branch from fd0621b to 3bc7e0e Compare June 6, 2024 07:44
@bartaz bartaz merged commit 426d151 into canonical:main Jun 6, 2024
6 checks passed
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.

Misleading icon import documentation
4 participants