lab.js icon indicating copy to clipboard operation
lab.js copied to clipboard

add - shortCut description : ctrl+s and ctrl+p

Open rbalet opened this issue 4 years ago • 2 comments

For : #60

rbalet avatar Dec 19 '19 09:12 rbalet

Hej, thanks a lot, that looks very good! I had a couple of random ideas that popped into my head as I was going through the code; let me know what you think, and whether you'd like to consider them for your PR -- otherwise I'll put them on my todo list:

  • Do you think it would make sense to have a new helper component that encapsulates some of the logic, for example something like <KeyboardTooltip target="..." key="s">Save</KeyboardTooltip> ? That component could then handle cross-platform differences internally (e.g. on Mac OS the shortcut would typically be displayed as ⌘S), and UX-wise I think it would be also cool to not just show the shortcut, but also a help hint.
  • Maybe you could also consider combining both the hint and the shortcut in a way that the shortcut is slightly muted in color, as we do with the module name here:
    Screenshot 2019-12-19 at 12 01 04

Finally, just as a hint: You know I'm a fan of prettier, and consistent code-formatting in general, and I'm trying to figure out how to implement that. That's for sure going to come sooner rather than later. For the meantime, it would make it massively easier for me to review your PRs if you could separate formatting and structural changes, for example by applying prettier only to your changed rows, or by breaking the autoformatting and the code changes into separate commits on the same branch. That way would make it much easier for me to see what's actually going on, and I can decide what to merge in the end. That's not strictly necessary here, but going forward I think that would be a massive help.

Ok, so much for now, hope this helps, and let's get this merged! Thanks again for your efforts!

-Felix

FelixHenninger avatar Dec 19 '19 11:12 FelixHenninger

@FelixHenninger For this pull request I deactivated my prettier autoformating '^^.
It have to be something else, I have to see what, but I'll do it like that with a complete free formater text editor.
For the rest (KeyboardTooltip) I can do it like that.

For thise one image do you already have it into your code base or it is something new?
(Do we have a react component that already exist or do I have to create something new?)

rbalet avatar Dec 21 '19 22:12 rbalet