Feature/icon partial #876
Merged
Feature/icon partial #876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the work needed before replacing IconComponent usages with iconPartial. Since there are many icon usages, it wouild be hard to see these changes if they were combined with the replacements. - register the relativePathHandler for SDK components so it can be used at runtime - fix bug in relativePathHandler where if the url is undefined, it will return something like "./undefined" instead of the value undefined. In previous usages of the helper, url was guarded from being null/undefined, but this won't be the case for iconPartial - add percy snapshots for a card with custom cta icons - fix test-sites gitignore negations not working properly - point the theme's test-site to the feature/icon-partial-i18n so we get the icon-partial change, and also i18n-ed assets so the spanish snapshots work - also collect code coverage on the hbshelpers folder J=SLAP-1297 TEST=manual,auto test that custom icons still work, both for section titles on universal and inside cards unit tests for relativePathHandler
This PR updates all IconComponent usages to use the icon partials in the SDK It also points the test-site to the feature/develop-i18n branch of the SDK, which is the same commit that develop is currently at, but includes i18n assets so our spanish snapshots work The first group of commits do a find and replace for identical (including whitespace like indentation) usages of IconComponent. J=SLAP-1297 TEST=manual test that custom icons can still be set for card CTAs, section titles, and the searchbar see that built in icons still show up
…th (#873) Noticed this while testing replacing all the IconComponent usages. Relative path was not being appended to the iconUrl correctly because the template was referencing just `relativePath` and not `@root.relativePath` or `../../relativePath` (i.e. it was using the wrong handlebars context). This wouldn't have caused visual issues for most users, since with the relativePath not being set correctly the iconUrl would get set to `/<iconUrl>`, instead of something like `../<iconUrl>` which for most cases will point to the same location. This only matters if somebody is taking the files output by the theme's build, and moving them into a directory structure. In that case things may break, since the two iconUrls above may no longer point to the same location. I changed the other two relativePath usages for consistency J=SLAP-1297 TEST=manual test that the iconUrl for alternativeverticals on a full page map now uses the correct relative url, instead of the absolute url that would result from relativePath being null added a percy snapshot for alternative verticals on the full page map, though this doesn't really test what was being fixed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.