Skip to content

Use Apple glyphs for Cmd, Alt, Ctrl and Shift #11

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

Closed
wants to merge 4 commits into from
Closed

Use Apple glyphs for Cmd, Alt, Ctrl and Shift #11

wants to merge 4 commits into from

Conversation

codedbypm
Copy link
Contributor

In this PR:

  • added keyboard shortcut for fix-its hint
  • replace all Cmd/Alt/Ctrl/Shift with apple glyphs ⌘ ⌥ ⌃ ⇧

@jessesquires
Copy link
Member

Thanks @codedbypm ! 💯 🎉

I definitely like this idea.

Some concerns:

  1. readability / accessibility. some of the glyphs (especially shift) seem quite small and difficult to read -- at least in the markdown. Maybe the rendered site will be different?
  2. We should probably use the html codes for these, right? For example, ⌘ which renders as ⌘

Maybe we use a hybrid approach, like this:

⌘J (cmd-J)

Copy link
Member

@jessesquires jessesquires left a comment

Choose a reason for hiding this comment

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

going to formally "request changes" as we discuss 😊

@codedbypm
Copy link
Contributor Author

codedbypm commented Mar 31, 2021

Thanks @codedbypm ! 💯 🎉

I definitely like this idea.

Some concerns:

  1. readability / accessibility. some of the glyphs (especially shift) seem quite small and difficult to read -- at least in the markdown. Maybe the rendered site will be different?

Correct. I also noticed that when editing the markdown, they are all a tad small, and shift is the one where the issue is more evident. Here you can see the rendered page. They are visually better than in the raw markdown

image

  1. We should probably use the HTML codes for these, right? For example, ⌘ which renders as ⌘

Yeah, HTML feels more future proof. It renders always properly (but not when text is in between backticks!) and allows for easier copy-paste or search-replace operations. For that, I would propose to add a commented-out legend at the beginning of the file to use as a quick reference for the random github user that passes by and wants to contribute and needs those glyphs.
I'll update the PR and await for your feedback.

Maybe we use a hybrid approach, like this:

⌘J (cmd-J)

mmm let me think about this last one

@jessesquires
Copy link
Member

jessesquires commented Mar 31, 2021

It renders always properly (but not when text is in between backticks!)

lol, oops I forgot about that. 😊

I'll update the PR and await for your feedback.

Sounds good. And I'm sorry about the conflicts! 😄


Edit: Oh one other thing, it looks like putting a space between characters also helps with readability.

For example: ⌘ J vs ⌘J

@codedbypm
Copy link
Contributor Author

It renders always properly (but not when text is in between backticks!)

lol, oops I forgot about that. 😊

I'll update the PR and await for your feedback.

Sounds good. And I'm sorry about the conflicts! 😄

Edit: Oh one other thing, it looks like putting a space between characters also helps with readability.

For example: ⌘ J vs ⌘J

It renders always properly (but not when text is in between backticks!)

lol, oops I forgot about that. 😊

I'll update the PR and await for your feedback.

Sounds good. And I'm sorry about the conflicts! 😄

Edit: Oh one other thing, it looks like putting a space between characters also helps with readability.

For example: ⌘ J vs ⌘J

I updated the PR

@codedbypm
Copy link
Contributor Author

codedbypm commented Apr 1, 2021

Has content been moved around recently? This PR has now merge conflicts that are not fixable by just one tap. And after spending the whole day at work, coding, reviewing, fixing conflicts, etc, the last thing I want in the eve is to have more conflicts to solve :D And from such a simple repo, I would definitely not expect conflicts

@jessesquires
Copy link
Member

Has content been moved around recently?

Yes. We renamed "Shortcuts" to "Keyboard Shortcuts" and re-alphabetized the list.

This PR has now merge conflicts that are not fixable by just one tap. And after spending the whole day at work, coding, reviewing, fixing conflicts, etc, the last thing I want in the eve is to have more conflicts to solve

Totally understandable. 🤗 That's why we call it open sores software. 😆

It might be easier to just re-branch off off main. However, it you do not feel up for doing more work here, I totally understand.

@jessesquires
Copy link
Member

@codedbypm I sent you an invite to join the org, which means you won't have to work off of a fork anymore, and should make development easier.

In the case that you do not want to move forward, I've opened #19 to track this so that someone else could carry this forward if they wish.

Again -- sorry about the conflicts! I know it's frustrating.

@codedbypm
Copy link
Contributor Author

@codedbypm I sent you an invite to join the org, which means you won't have to work off of a fork anymore, and should make development easier.

In the case that you do not want to move forward, I've opened #19 to track this so that someone else could carry this forward if they wish.

Again -- sorry about the conflicts! I know it's frustrating.

No worries. That night was the end of a very long coding day :D My complaint had only the goal to make things better and smoother for whoever comes next.
I accepted your invite (thx btw) and will send soon a new PR based on the current main. It will be faster than fixing conflicts :D

@codedbypm
Copy link
Contributor Author

Closing this in favor of #20

@codedbypm codedbypm closed this Apr 3, 2021
@jessesquires
Copy link
Member

My complaint had only the goal to make things better and smoother for whoever comes next.

I think #16 could help with this. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants