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

test: use optimized assets #1354

Merged
merged 4 commits into from
Mar 29, 2024
Merged

test: use optimized assets #1354

merged 4 commits into from
Mar 29, 2024

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Mar 22, 2024

When testing Ionicons manually there was an issue with our configuration where the index.html file served by the Stencil dev server never used the optimized SVG assets.

We've had issues in the past where the optimization script incorrectly changes the behavior of an SVG, so being able to validate that this script is working as intended is important.

The index.html used in testing loads asset from the build directory. The build script does copy optimized SVG assets to the build directory. The problem is that this causes the Stencil Dev Server to re-build. As part of the Stencil config we instruct it to copy all unoptimized SVGs to the build directory.

As a result, we have the following flow:

  1. Build script runs.
  2. Optimized SVGs are copied to build/src.
  3. Stencil dev server rebuilds.
  4. Unoptimized SVGs are copied to build/src overriding the optimized SVGs.

I've made the following changes to correct this:

  1. Removed the SVG copy step from the Stencil config.
  2. Updated the start script to optimize all SVG assets and copy them to the build directory. This ensures that optimized SVGs are being generated every time npm run start runs. Note that Stencil will not re-run this optimization step on each re-build. Devs need to continue to run npm run build.files after the initial build if they want to modify icons and see their changes.

Testing:

  1. Run npm run start
  2. On the testing page that loads, verify that svg elements inside of the Shadow DOM for ion-icon have the ionicon class.
  3. Run npm run build.files. Repeat step 2.

Note that on main, the ionicon class will not be present on the SVG elements indicating that the unoptimized SVG is being loaded.

The screenshot diffs here are correct and were capturing the original buggy behavior. The call (@) icon has color="tertiary" set and should always have been a cyan color. The heart outline color had --ionicon-stroke-width: 8px as opposed to the default 32px and should always have been thinner than normal.

@liamdebeasi liamdebeasi changed the title Fix build test: testing uses optimized assets Mar 24, 2024
@liamdebeasi liamdebeasi changed the title test: testing uses optimized assets test: use optimized assets Mar 24, 2024
@liamdebeasi liamdebeasi marked this pull request as ready for review March 24, 2024 21:35
@liamdebeasi liamdebeasi requested review from a team and sean-perkins and removed request for a team March 24, 2024 21:35
Copy link
Contributor

@mapsandapps mapsandapps left a comment

Choose a reason for hiding this comment

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

Great! As well as it fixing the issues with your reproduction steps, it also fixes the issue I was seeing with using color on -outline icons. Thanks for fixing this!

@liamdebeasi liamdebeasi merged commit 71938d0 into main Mar 29, 2024
6 checks passed
@liamdebeasi liamdebeasi deleted the fix-build branch March 29, 2024 15:24
christian-bromann pushed a commit to christian-bromann/ionicons that referenced this pull request May 1, 2024
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.

None yet

3 participants