Kully
add shortcut legend
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.
@jkl3848 Good idea and would probably be helpful.
I have a couple comments:
- We should probably allow it to be collapsable.
- 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?
@jkl3848 Hey just checking in? Are you still tryin to get this merged (and the other one you started #29 )
@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.
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 :)
Just to make sure I understand, you're saying just keep those three in the legend, since the others are already in tooltips?
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 https://github.com/Kully/pixel-paint/pull/28#issuecomment-2388781300, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACPDQR46SMUXRGDDA5EOPADZZP6UPAVCNFSM6AAAAABIPXP6ICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBYG44DCMZQGA . You are receiving this because you commented.Message ID: @.***>
@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 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. 🙌
@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 👍🏻
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 😛)