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

[Explorer] Reduces the size of the copy icon that is used with IDs #4504

Merged
merged 12 commits into from
Sep 9, 2022

Conversation

apburnie
Copy link
Contributor

@apburnie apburnie commented Sep 7, 2022

  • Reflecting the scarcity of copy icons in the new Figma designs, the default for the Longtext component is now no copy button.
  • Two copy buttons are taken from the Figma designs: 16 x 16 and 24 x 24 pixels.
  • The copy icons used for the IDs in headers are swapped for the 24 x 24 version.
  • The copy icons used for IDs elsewhere are swapped for the 16 x 16 version. This shrinks most copy icons.
  • When examining the website on a smart phone, the 16 x 16 icons disappear.

Here's a video showing how these changes play out across the website:

firefox_Kngum7HkWh.mp4

The above description has been updated as of 8 September 2022

@apburnie apburnie linked an issue Sep 7, 2022 that may be closed by this pull request
@apburnie apburnie marked this pull request as ready for review September 7, 2022 10:54
@Jibz1
Copy link
Contributor

Jibz1 commented Sep 7, 2022

@Andrew47 the ✔️success seems off

@mystie711
Copy link
Collaborator

Figma reference,
https://www.figma.com/file/kSD0qXCIDZA4KQVNepA5hR/02-Explorer?node-id=158%3A35722

The proportions seem off within the badge.
These kind of misalignments usually happen because of the icon shape is chosen when exporting assets vs the icon container. One good way to always ensure you are exporting the correct asset is to look for a perfect square proportion on export.

Copy link
Collaborator

@mystie711 mystie711 left a comment

Choose a reason for hiding this comment

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

@Andrew47 just a few more pixel pushes and this should be ready to go.
image

@apburnie
Copy link
Contributor Author

apburnie commented Sep 9, 2022

@mystie711 --> Does this work?

image

@mystie711
Copy link
Collaborator

mystie711 commented Sep 9, 2022

@Andrew47 the text alignment looks better. But now the check icon looks off center vertically by a pixel. May need to handle their placements individually?

Let's ship this for now. I might need to do some adjustments to the icon itself in future.

Copy link
Contributor

@stella3d stella3d left a comment

Choose a reason for hiding this comment

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

code looks good!
only thing i noticed in the video is that we get overlapping text when the UI is very narrow, as shown here
image

If that's caused by this PR, we should fix it before merge ideally - if not, we can fix in another PR.

@apburnie apburnie dismissed mystie711’s stale review September 9, 2022 14:39

Agreement to ship given

@apburnie apburnie merged commit a8dafe7 into main Sep 9, 2022
@apburnie apburnie deleted the copyIcon branch September 9, 2022 14:39
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.

Explorer: Copy icon is bigger than it should be
4 participants