feat(nav): Updated search map title & icon #1894
Merged
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
…ssue the height and width on the raw SVG file were both set to 24 (matching the viewbox definition). Surprisngly, the image was being cut off. I found that removing the height/width properties resolved the issue. Looking further into it, the viewport was being removed by svgo - see [this](svg/svgo#1128) extensively discussed issue. Since we are re-scaling the svg with CSS to 20x20, we do need to preserve the image viewbox. Applying the solution from [this](svg/svgo#1128 (comment)) comment
Going through all other icons to see which had a viewbox that matched the width + height and found only two: icon-garage and icon-up-right-arrow. icon-up-right-arrow had sizing explicitly set in CSS and was not affected. icon-garage defaulted to the leaflet default marker size of 12x12. This explicitly sets the iconSize to 21x25, matching what is specified in the SVG. I noticed that the width and height SVG properties are being removed as part of [svg-inline-loader](https://github.com/webpack-contrib/svg-inline-loader/tree/4bb5529fbdd847d7809067fe11840b48ee13dde4#removesvgtagattrs-boolean). I thought about turning off that behavior as part of this PR, but it seems like it could affect more icons and I'm not really sure if it is necessary
Coverage of commit
|
lemald
reviewed
Jan 24, 2023
| @@ -215,6 +217,8 @@ export const SaveIcon = svgIcon(saveIconSvg) | |||
|
|
|||
| export const SearchIcon = svgIcon(searchIconSvg) | |||
|
|
|||
| export const SearchMapIcon = svgIcon(searchMapIconSvg) | |||
There was a problem hiding this comment.
suggestion: it's probably worth adding this icon to the list in icon.test.tsx, even if I have my doubts about how useful those particular tests are.
lemald
approved these changes
Jan 24, 2023
firestack
approved these changes
Jan 24, 2023
There was a problem hiding this comment.
Oh neat! I wonder if 212519f (#1894) might fix the logo icon root scaling issue I encountered.
It seems like it should, but when I tried it locally somehow the skate logo is still getting it's viewbox stripped (after I exported the logo from the component library which had a viewbox on it, where our asset in the repo does not).
Coverage of commit
|
Coverage of commit
|
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.
ticket: https://app.asana.com/0/1200180014510248/1203548727085588
A few notes on this PR:
I thought about generalizing the other modes (ladder page, settings, shuttle map) to also use the
NavModeinterface so that searchMap isn't a special case inpageOrTabName. It seems doable and could also potentially simplify the router with something like a route-config file. That seems to be pushing the scope of this ticket. If reviewers are interested I can timebox incorporating that into this PR or create a tech-debt ticket for it.Commits
4c37a32212519f and 4c37a32 both have more detailed commits messages that provide some extra context for those changes.