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

Update EIP-6963: make icon requirement wording consistent #7790

Merged
merged 1 commit into from Oct 4, 2023

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Sep 27, 2023

Noticed this inconsistency with SHOULD and MUST for the icon aspect ratio and resolution. I believe this should be SHOULD

@jiexi jiexi requested a review from eth-bot as a code owner September 27, 2023 21:28
@github-actions github-actions bot added c-status Changes a proposal's status s-review This EIP is in Review t-interface labels Sep 27, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Sep 27, 2023

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Sep 27, 2023
@kdenhartog
Copy link
Contributor

kdenhartog commented Sep 27, 2023

The way I interpret the MUST is that "it MUST be 96 x 96 or larger" and it SHOULD be a square. AKA

95x95 is not allowed
97x97 is allowed and expected
97x96 is allowed, but not expected

I think what's written is a bit confusing as I think the intention here is to only allow the second case and exclude the 3rd because it's not a square.

@bpierre
Copy link

bpierre commented Sep 28, 2023

I think it would be preferable to have both requirements as MUST, so that UI authors can rely on the icon to be squared.

@jiexi
Copy link
Contributor Author

jiexi commented Sep 28, 2023

I don't think we have consensus on this one yet. So will hold off on making any changes

Copy link
Contributor

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

Seems like we don't have any strong opposition to this change in the TG channel. This language seems more consistent now as well.

@eth-bot eth-bot enabled auto-merge (squash) October 4, 2023 23:16
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit 946735d into ethereum:master Oct 4, 2023
10 checks passed
@jiexi jiexi deleted the jl/eip-6963-icon-must-wording branch October 4, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-status Changes a proposal's status s-review This EIP is in Review t-interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants