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

Icon keyboard shortcut #2413

Merged
merged 6 commits into from
Oct 11, 2019
Merged

Conversation

mdefazio
Copy link
Contributor

@mdefazio mdefazio commented Oct 10, 2019

Summary

Add keyboard shortcut icon. Closes #1573

Dark mode:
image

Light mode:
image

Checklist

  • Checked in dark mode
  • [ ] Checked in mobile
  • [ ] Checked in IE11 and Firefox
  • [ ] Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Looks good from the screenshots!
Can you post a pixel-level detailed shot of the icon just to see how it aligns to the 16x16px grid?

Thanks!

@@ -0,0 +1,3 @@
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16" viewBox="0 0 16 16">
<path fill-rule="evenodd" clip-rule="evenodd" d="M1 1H15V6H1V1ZM0 1C0 0.447715 0.447715 0 1 0H15C15.5523 0 16 0.447715 16 1V6C16 6.55228 15.5523 7 15 7H1C0.447715 7 0 6.55228 0 6V1ZM2 4H8V5H2V4ZM3 10H2V12H-3.8147e-06V13H2V15H3V13H5V12H3V10ZM14 13H8V14H14V13ZM15 10H7V15H15V10ZM7 9C6.44772 9 6 9.44771 6 10V15C6 15.5523 6.44772 16 7 16H15C15.5523 16 16 15.5523 16 15V10C16 9.44772 15.5523 9 15 9H7Z"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to just manually remove the fill-rule and clip-rule

Copy link
Contributor

Choose a reason for hiding this comment

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

This was the rare case where it was actually needed believe it or not. It blocked out the content without it. (I was watching over @mdefazio's shoulder)

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't look too deep at the SVG itself though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll peek at the Figma file too, but doesn't seem like a show stopper

Copy link
Contributor

@ryankeairns ryankeairns Oct 10, 2019

Choose a reason for hiding this comment

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

I tried flattening and unioning into one shape to no avail in Figma, so I gave it a go with Sketch and came out with a much cleaner svg than what I was able to produce with Figma:

<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
  <path d="M15,9 C15.51285,9 15.9355092,9.38604429 15.9932725,9.88337975 L16,10 L16,14 C16,14.51285 15.613973,14.9355092 15.1166239,14.9932725 L15,15 L8,15 C7.48716857,15 7.06449347,14.613973 7.0067278,14.1166239 L7,14 L7,10 C7,9.48716857 7.38604429,9.06449347 7.88337975,9.0067278 L8,9 L15,9 Z M2.5,10 C2.77614,10 3,10.2239 3,10.5 L3,12 L4.5,12 C4.77614,12 5,12.2239 5,12.5 C5,12.7761 4.77614,13 4.5,13 L3,13 L3,14.5 C3,14.7761 2.77614,15 2.5,15 C2.22386,15 2,14.7761 2,14.5 L2,13 L0.5,13 C0.223858,13 0,12.7761 0,12.5 C0,12.2239 0.223858,12 0.5,12 L2,12 L2,10.5 C2,10.2239 2.22386,10 2.5,10 Z M15,10 L8,10 L8,14 L15,14 L15,10 Z M13.5,12 C13.7761,12 14,12.2239 14,12.5 C14,12.7454222 13.8230914,12.9496 13.5898645,12.9919429 L13.5,13 L9.5,13 C9.22386,13 9,12.7761 9,12.5 C9,12.2545778 9.17687704,12.0504 9.41012499,12.0080571 L9.5,12 L13.5,12 Z M15,1 C15.51285,1 15.9355092,1.38604429 15.9932725,1.88337975 L16,2 L16,7 C16,7.51283143 15.613973,7.93550653 15.1166239,7.9932722 L15,8 L1,8 C0.487163929,8 0.0644928061,7.61395571 0.00672772777,7.11662025 L0,7 L0,2 C0,1.48716857 0.386039974,1.06449347 0.883378828,1.0067278 L1,1 L15,1 Z M15,2 L1,2 L1,7 L15,7 L15,2 Z M8.5,5 C8.77614,5 9,5.22386 9,5.5 C9,5.77614 8.77614,6 8.5,6 L2.5,6 C2.22386,6 2,5.77614 2,5.5 C2,5.22386 2.22386,5 2.5,5 L8.5,5 Z"/>
</svg>

Copy link
Contributor

Choose a reason for hiding this comment

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

Sketch seems to do a better job of creating a combined shape. I also went into the rectangles and set the punch out layers to do a 'subtract'. For whatever reason, that didnt produce the same outcome in Figma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back in and re-exported from figma and sketch. As we've discussed, the sketch file was slightly off in a few places.
Updated the svg code:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16" viewBox="0 0 16 16">
  <path d="M15 9a1 1 0 01.993.883L16 10v5a1 1 0 01-.883.993L15 16H7a1 1 0 01-.993-.883L6 15v-5a1 1 0 01.883-.993L7 9h8zM2.5 10a.5.5 0 01.492.41L3 10.5V12h1.5a.5.5 0 01.09.992L4.5 13H3v1.5a.5.5 0 01-.992.09L2 14.5V13H.5a.5.5 0 01-.09-.992L.5 12H2v-1.5a.5.5 0 01.5-.5zM15 10H7v5h8v-5zm-1 3v1H8v-1h6zm1-13a1 1 0 011 1v5a1 1 0 01-1 1H1a1 1 0 01-1-1V1a1 1 0 011-1h14zm0 1H1v5h14V1zM8 4v1H2V4h6z" />
</svg>

@mdefazio
Copy link
Contributor Author

Screenshot from Figma with 1x1 pixel on 16x16 frame:
image

@ryankeairns ryankeairns self-requested a review October 11, 2019 16:31
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Lookin' good!

It looks like you'll need to update the snapshot. You can do that by running yarn run test-unit -- -u and committing the result (i.e. the updated snapshot file).

@mdefazio mdefazio merged commit df5b1b2 into elastic:master Oct 11, 2019
@mdefazio mdefazio deleted the icon-keyboard-shortcut branch October 11, 2019 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add keyboard shortcut icon
3 participants