add shortcut legend

[jkl3848]

I thought it might be handy to have the keyboard shortcuts list on the page itself, instead of having to reference the git repo. I tried to keep the style simple.

[Kully]

@jkl3848 Good idea and would probably be helpful.

I have a couple comments:

1. We should probably allow it to be collapsable.
2. It right now sits too far low. We want to place it in a more accessible place.

• Maybe it makes sense to fix its position to the top left?
• We could also hide it away and open it in a modal when you click on an "info" button?

Maybe something like this? What do you think?

[Kully]

@jkl3848 Hey just checking in? Are you still tryin to get this merged (and the other one you started #29 )

[jkl3848]

@jkl3848 Hey just checking in? Are you still tryin to get this merged (and the other one you started #29 )

Sorry I've been MIA. Been busy with some other projects. Yes I hope to finish these up soon.

[Kully]

Sorry I've been MIA. Been busy with some other projects. Yes I hope to finish these up soon.

Sounds good! No worries, we are all busy people 🙏

Reviewing the PR again, it already looks like we are indicating the shortcuts for some of the commands: if you hover over some of the buttons, it shows you the commands.

example:

But some of them are not indicated, as you have indicated in the legend.

To not repeat ourselves, we could maybe just keep these 3 in the shortcut:

Curious what you think :)

[jkl3848]

Just to make sure I understand, you're saying just keep those three in the legend, since the others are already in tooltips?

[Kully]

Yes. Thoughts?

…

On Wed, Oct 2, 2024 at 4:23 PM Jachin ***@***.***> wrote: Just to make sure I understand, you're saying just keep those three in the legend, since the others are already in tooltips? — Reply to this email directly, view it on GitHub <#28 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACPDQR46SMUXRGDDA5EOPADZZP6UPAVCNFSM6AAAAABIPXP6ICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBYG44DCMZQGA> . You are receiving this because you commented.Message ID: ***@***.***>

[jkl3848]

Sounds good. I'll make the change

[jkl3848]

@Kully Ok I removed the extra shortcuts and switched to a onHover popover. Let me know what you think.

I dont mind replacing the word with an icon, I just wasn't sure what you use for icons.

[Kully]

@Kully Ok I removed the extra shortcuts and switched to a onHover popover. Let me know what you think.

I dont mind replacing the word with an icon, I just wasn't sure what you use for icons.

Thanks for removing the extra shortcuts.

I felt the placement on the screen however was a little unintuitive in that it is not obvious that something will happen when you hover or click on it (not your fault, I didn't give a design requirement for this task).

I did some tweaking in CSS. Do you mind applying these changes? Once you do, we should be ready to merge. 🙌

[jkl3848]

@Kully Ok I removed the extra shortcuts and switched to a onHover popover. Let me know what you think.
I dont mind replacing the word with an icon, I just wasn't sure what you use for icons.

Thanks for removing the extra shortcuts.

I felt the placement on the screen however was a little unintuitive in that it is not obvious that something will happen when you hover or click on it (not your fault, I didn't give a design requirement for this task).

I did some tweaking in CSS. Do you mind applying these changes? Once you do, we should be ready to merge. 🙌

LGTM 👍🏻

[Kully]

LGTM 👍🏻

Let me know if you are able to make these changes and we can go from there.

(PS I don't think I have permissions to, otherwise I would have done it 😛)

[jkl3848]

@Kully Ok should be good to go

[Kully]

@Kully Ok should be good to go

Superb! Thank you!! 💃

[Kully]

LGTM!