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

Add shortcuts and tooltips to forms icons #183

Closed
4 tasks done
tyler-dane opened this issue Dec 11, 2024 · 8 comments · Fixed by #343
Closed
4 tasks done

Add shortcuts and tooltips to forms icons #183

tyler-dane opened this issue Dec 11, 2024 · 8 comments · Fixed by #343
Assignees

Comments

@tyler-dane
Copy link
Contributor

tyler-dane commented Dec 11, 2024

User Story

As a user, I want to see a tooltip when hovering over the icons in the event form, so that I can quickly understand their functionality before clicking them.

Acceptance Criteria

  • Adds meta icon from Phosphor icons command for Mac and windows-logo for Windows
  • Add tooltip to trash icon in both the EventForm and SomedayEventForm, where the description is Delete and the shortcut is DEL
  • Add tooltip to the < icon in the EventForm, where the description is Move to sidebar and the shortcut is [META] + SHIFT + <. Replace [META] with the correct meta icon for the platform (windows or command)
  • Add unit test to confirm the shortcut triggers the correction action

Implementation Guidance

  • use react-hotkeys-hook

Context

  • For delete, use the native Delete (Key.Delete from ts-key-enum). d is needed for regular event titles/descriptions and [META] + DELETE is the OS default for deleting characters behind a cursor.
@tyler-dane tyler-dane moved this to Backlog in 🏗 Compass Roadmap Dec 11, 2024
@tyler-dane tyler-dane added the needs more info Not ready for coding just yet label Dec 11, 2024
@murilo9
Copy link
Contributor

murilo9 commented Dec 13, 2024

I'll start working on this one.

@tyler-dane
Copy link
Contributor Author

@murilo9 This one isn't ready to be worked on. That's why it has the needs more info tag and is in the backlog column, rather than the Ready column.

You said before you were working on #165 - can you finish that up instead?

@tyler-dane tyler-dane self-assigned this Jan 24, 2025
@tyler-dane tyler-dane changed the title Delete event with shortcut Delete event with keyboard Jan 24, 2025
@tyler-dane tyler-dane changed the title Delete event with keyboard Add tooltip to delete icon in forms Jan 27, 2025
@tyler-dane tyler-dane changed the title Add tooltip to delete icon in forms Add shortcuts and tooltips to forms icons Jan 27, 2025
@tyler-dane tyler-dane removed the needs more info Not ready for coding just yet label Jan 27, 2025
@tyler-dane tyler-dane moved this from Backlog to Ready in 🏗 Compass Roadmap Jan 27, 2025
@that-one-arab
Copy link
Contributor

@tyler-dane Thanks to the span's title prop, we get a native tooltip displayed when hovered over the button, would that suffice

Image

@tyler-dane
Copy link
Contributor Author

Yes that's fine for now

@that-one-arab
Copy link
Contributor

@tyler-dane I remembered that we also need to implement images so I had to switch that with a better implementation that supports images, LMK what you think

@tyler-dane
Copy link
Contributor Author

The conditional meta icon seems to be working well.

Small request:

  • update the label to include a space between the + and the <

Also, the shortcut isn't actually doing anything. I think that's expected (?). We'd have to register it in useShortcuts so that we can dispatch the convert action.

Can you please create an issue for that and do it after #282?

not.doing.anything.mov

@that-one-arab
Copy link
Contributor

@tyler-dane Thank you for your input.

I tested the shortcut using combination META + shift + (letter accessible on my keyboard and does not need me to hold shift)

So I tested using combination (META + shift + A) and combination (META + shift + S) and both worked, so I do not believe we need to update useShortcuts

I could not test using combination (META + shift + <) because the "<" sign because it needs me to hold shift to press it.
I use a Turkish keyboard layout, so I thought that due to a keyboard layout difference, you and US based users would be able to test it (I guess, the "<" choice in the combination was confusing to me)

But I should have tested it or communicated my confusion with the "<" selection, that's on me.

I suggest that we change the input into something more accessible. Please let me know what you think.

@tyler-dane
Copy link
Contributor Author

Yeah I think we gotta change it to something that'll work for more people, but is also somewhat intuitive.

What about any of these

META + {leftArrow}
META + ,
META + j (since j is used to navigate to previous week, maybe that'll make sense to ppl)
META + [ (since [ is used for sidebar toggling, they might associate it with the sidebar more easily)

Whichever you go with, plz be sure to double-check that it doesn't conflict with native browser or OS behavior

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

Successfully merging a pull request may close this issue.

3 participants