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] Update new SVG file and snapshots for Polygon #1139

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

willie-hung
Copy link
Contributor

@willie-hung willie-hung commented Nov 2, 2023

Description

Updated new polygon icon svg

Issues Resolved

Resolve #1068

Screenshots

After

Screenshot 2023-11-02 at 1 35 55 PM Screenshot 2023-11-02 at 1 35 41 PM

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@willie-hung willie-hung changed the title Update new SVG file and snapshots [Bug] Update new SVG file and snapshots Nov 2, 2023
@willie-hung willie-hung changed the title [Bug] Update new SVG file and snapshots [Fix] Update new SVG file and snapshots for Polygon Nov 2, 2023
Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
@BSFishy BSFishy added the OSCI label Nov 2, 2023
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

@Willie-The-Lord Check the updated renderedSVG output in the "Changed Files" tab - it looks like something still isn't quite right, as the boxes are mostly filled instead of empty. It may be the fillRule or some other missing property. You may want to check similar icons with enclosed areas to see what the issue is.

@willie-hung
Copy link
Contributor Author

You may want to check similar icons with enclosed areas to see what the issue is.

@joshuarrrr, just updated the polygon svg. Thanks for the heads up.

src/components/icon/assets/polygon.svg Outdated Show resolved Hide resolved
src/components/icon/assets/polygon.svg Outdated Show resolved Hide resolved
willie-hung and others added 5 commits December 7, 2023 11:31
Co-authored-by: Matt Provost <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
Co-authored-by: Matt Provost <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
@willie-hung
Copy link
Contributor Author

Thanks @BSFishy! Based on these two sources: clip-rule & fill-rule. I think we can remove the clip-rule and remain the fill-rule.

@joshuarrrr
Copy link
Member

joshuarrrr commented Dec 7, 2023

@Willie-The-Lord Thanks so much for your patience and persistence here.

I'm a little torn about approving this and merging as is (because I do believe it fixes #1068), or continuing to iterate.

The pedantic side of me wonders why we're introducing the first icon defined with a 0 0 48 48 viewbox definition. Most other icons are defined with a 0 0 16 16 viewbox, and the rest with 0 0 32 32 (see https://github.com/search?q=repo%3Aopensearch-project%2Foui+viewbox%3D+language%3ASVG+path%3A%2F%5Esrc%5C%2Fcomponents%5C%2Ficon%5C%2Fassets%5C%2F%2F&type=code). The fact that we're diverging from that pattern is an indicator to me that we may want to revisit https://github.com/opensearch-project/oui/blob/main/wiki/creating-icons.md to be more explicit about viewbox vs width and height props.

@willie-hung
Copy link
Contributor Author

@joshuarrrr I completely agree. It seems like there is no clear instruction on how to defineviewbox, height and width when creating new icons. Should we involve the UX team in this discussion?

@AMoo-Miki
Copy link
Collaborator

The thickness of the lines is more than other 16x16 icons. I feel the original was just scaled while it should have been redrawn.

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.

[BUG] Polygon. Icon bug
4 participants