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.
Uh oh!
There was an error while loading. Please reload this page.
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.
The AI model I used told me
144 -816 672 672is the viewbox that fits the path perfectly which I used in #34517.The original viewbox was directly from the Material icons which didn't fit the path perfectly so it became off-center depending on the font size.
Did you check the alignment when increasing/decreasing base browser font size via settings?
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.
Yea. Seems fine. The font size doesn't matter. Doesn't change. That's up to the button to align itself. If the button comes out of alignment then that's a different issue.
But I mean alignment of the rest of the row is messed up because things like the checkboxes should be buttons etc.
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.
Well, it does matter if the path is not centered in viewbox. But seems like it still is.
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.
Yea I'm not sure where the 820 box came from because all the materialize design ones have a 960 height for the 24px versions which puts them center:
https://fonts.google.com/icons?selected=Material+Symbols+Outlined:close:FILL@0;wght@400;GRAD@0;opsz@24&icon.size=24&icon.color=%231f1f1f
Then I just applied equal sizing to make it a little larger while still always fitting in our viewbox. Seems to work well for all of them.
Really it's weird that we use 24px basis icons and then resize them to 16px. We should probably use 16px basis icon to begin with or maybe use 24px icons everywhere without having to scale them and fix the padding of the buttons instead.