Improve hotkey rendering in documentation #1479

Niloth-p · 2024-04-11T18:55:37Z

What does this PR do, and why?

Improves the rendering of hotkeys in hotkeys.md.

Make lowercase alphabet keys display as uppercase to match their physical representation in a keyboard.
Added a prefix of Shift to uppercase alphabet keys
Use the new display_keys_for_command() helper function in keys.py
- Space-separated keys that were previously being rendered separately, are now rendered as a single key
- Special key names are capitalized
Added arrow symbols to the direction keys

Outstanding aspect(s)

Tests are not included.

External discussion & connections

Fully fixes Improve key rendering in documentation #945

How did you test this?

Manually - Visual changes

Visual changes

Niloth-p · 2024-04-11T19:05:12Z

@zulipbot add "area: hotkeys" "PR needs review"

zulipbot · 2024-04-11T19:23:17Z

Hello @zulip/server-hotkeys members, this pull request was labeled with the "area: hotkeys" label, so you may want to check it out!

neiljp

@Niloth-p Thanks for this matching PR to #1476 👍

I replied about this in the stream, but left some comments inline too, which will apply depending on how we move forward with this.

neiljp · 2024-04-12T19:14:04Z

zulipterminal/config/symbols.py

+LEFT_ARROW = "←"  # LEFTWARDS ARROW, U+2190
+UP_ARROW = "↑"  # UPWARDS ARROW, U+2191
+RIGHT_ARROW = "→"  # RIGHTWARDS ARROW, U+2192
+DOWN_ARROW = "↓"  # DOWNWARDS ARROW, U+2193


I'm inclined to keep this change as something that we choose to do universally, rather than only in the hotkeys doc.

We'll definitely want a space between the text and symbols if we do this.

I'm inclined to keep this change as something that we choose to do universally, rather than only in the hotkeys doc.

We'll definitely want a space between the text and symbols if we do this.

Just to clarify,
We're updating all direction keys (and just the direction keys) to be - text + space + symbol.
Direction keys that are used in keys.py, in hotkeys.md and the ones in core.py (popup headers).

My main point was that we'll likely want to leave the symbol change to a different PR since I'm not sure we want it only in the docs. Specific symbols can be a separate discussion too - I recall some from the original PR weren't that clear.

Then, with the original changes you made I noticed that there wasn't a space, so for that future work the second comment I made was to indicate that we'll want a space if/when we do that.

neiljp · 2024-04-12T20:31:33Z

tools/lint-hotkeys

+                            f"<kbd>Shift</kbd> + <kbd>{key}</kbd>"
+                            if len(key) == 1 and key.isupper()
+                            else f"<kbd>{key.capitalize()}</kbd>"
+                            if len(key) == 1
+                            else f"<kbd>{key + DIRECTION_TO_SYMBOL_MAP.get(key, '')}</kbd>"
+                            for key in key_combination.split()


I replied separately in the stream, so we may not need this change as it stands.

However, when inline conditionals get large they get more difficult to read, so at a minimum I'd go with something like the following:

Suggested change

f"<kbd>Shift</kbd> + <kbd>{key}</kbd>"

if len(key) == 1 and key.isupper()

else f"<kbd>{key.capitalize()}</kbd>"

if len(key) == 1

else f"<kbd>{key + DIRECTION_TO_SYMBOL_MAP.get(key, '')}</kbd>"

for key in key_combination.split()

f"<kbd>Shift</kbd> + <kbd>{key}</kbd>"

if len(key) == 1 and key.isupper()

else (

f"<kbd>{key.capitalize()}</kbd>"

if len(key) == 1

else f"<kbd>{key + DIRECTION_TO_SYMBOL_MAP.get(key, '')}</kbd>"

)

for key in key_combination.split()

However, while it requires rewriting the comprehensions/loops, it may well be clearer to simply express it as regular if statements instead - particularly given one of the inline conditionals is itself a +-separated string.

Uses the new display functions from keys.py. Applies - Capitalization of special keys. - Fix "<kbd>page</kbd> + <kbd>up</kbd>" to "<kbd>PgUp</kbd>". Fixes zulip#945.

Tests updated.

Niloth-p

Commit Flow:

Apply the display functions from keys.py.
Append direction key symbols - in UI and docs.
Update the scroll prompt in Popup headers.
Use Shift in docs (for demo). Created functions and refactored the formatting.

Also, some miscellaneous changes,

Added a --fix argument in error message.
Added a docstring to a function in lint-hotkeys, for full coverage.

The commit structure is temporary.
I've just submitted the changes, I'll need to re-order them once we choose the parts we want, and group them.

Visual Changes

Help Menu

Popup Header

tools/lint-hotkeys

Niloth-p · 2024-04-13T08:12:06Z

tools/lint-hotkeys

+    for symbol in ARROW_SYMBOLS:
+        if symbol in key_combination:
+            return f"<kbd>{key_combination}</kbd>"
+    return " + ".join([format_key(key) for key in key_combination.split()])


Just added for an alternative option.
Also, there's the limitation that format_key_combination() does not work when the arrow key is used with modifier keys.

Re the point about the arrow keys, again let's keep that for building later.

One issue here is how we're defining what a 'key' is for various purposes, ie. surrounding by <kbd> tags as a keyboard-key, or as a key-combination. Currently the display function just gives a string, but we're not limited to that data format until we ultimately must output it. Also, if we style the keys (physical keyboard and combinations) in the UI, we'll likely want to think of how we structure them in a similar way.

tools/lint-hotkeys

Niloth-p · 2024-04-13T08:15:14Z

zulipterminal/config/symbols.py

+LEFT_ARROW = "←"  # LEFTWARDS ARROW, U+2190
+UP_ARROW = "↑"  # UPWARDS ARROW, U+2191
+RIGHT_ARROW = "→"  # RIGHTWARDS ARROW, U+2192
+DOWN_ARROW = "↓"  # DOWNWARDS ARROW, U+2193


I'm inclined to keep this change as something that we choose to do universally, rather than only in the hotkeys doc.

We'll definitely want a space between the text and symbols if we do this.

Just to clarify,
We're updating all direction keys (and just the direction keys) to be - text + space + symbol.
Direction keys that are used in keys.py, in hotkeys.md and the ones in core.py (popup headers).

Niloth-p · 2024-04-13T08:22:27Z

zulipterminal/config/symbols.py

+UP_ARROW = "↑"  # UPWARDS ARROW, U+2191
+RIGHT_ARROW = "→"  # RIGHTWARDS ARROW, U+2192
+DOWN_ARROW = "↓"  # DOWNWARDS ARROW, U+2193
+ARROW_SYMBOLS = [LEFT_ARROW, UP_ARROW, RIGHT_ARROW, DOWN_ARROW]


For convenient use in lint-hotkeys to check if the key_combination contains any of the arrow keys.

Replace the hardcoded scroll hints with the commands' display keys.

Prepends upper case alphabets with 'Shift + '.

zulipbot · 2024-04-14T06:33:00Z

Heads up @Niloth-p, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

neiljp

@Niloth-p Thanks for splitting out the core changes - I just merged the first commit as d65cbee 🎉

Regarding the remainder, I left inline comments.

For reviewing, it's good to have separate changes in separate commits except for the earliest code demos. For now, let's restructure the remainder to have the capitalization first, symbols last (if left in for others to view), and any adjustment to the linting tool in one or more separate commits.

Leaving the symbols until last, which are more of an unknown, means we can drop the commit at the end without needing to adjust the intervening commits for conflicts.

Having just been able to test it, the use of the common text in the menu titles looks promising as a refactor, but less-so with the symbols with the text. Without the symbols it looks better, but the upper case text from the capitalization then dominates the menu titles? I do think that refactor would be a good improvement, if not using the new key casing yet for this reason. It could then lead onto the popup general key improvements.

tools/lint-hotkeys

neiljp · 2024-04-14T03:55:55Z

zulipterminal/config/symbols.py

+LEFT_ARROW = "←"  # LEFTWARDS ARROW, U+2190
+UP_ARROW = "↑"  # UPWARDS ARROW, U+2191
+RIGHT_ARROW = "→"  # RIGHTWARDS ARROW, U+2192
+DOWN_ARROW = "↓"  # DOWNWARDS ARROW, U+2193


My main point was that we'll likely want to leave the symbol change to a different PR since I'm not sure we want it only in the docs. Specific symbols can be a separate discussion too - I recall some from the original PR weren't that clear.

Then, with the original changes you made I noticed that there wasn't a space, so for that future work the second comment I made was to indicate that we'll want a space if/when we do that.

tools/lint-hotkeys

neiljp · 2024-04-14T04:54:02Z

tools/lint-hotkeys

+    for symbol in ARROW_SYMBOLS:
+        if symbol in key_combination:
+            return f"<kbd>{key_combination}</kbd>"
+    return " + ".join([format_key(key) for key in key_combination.split()])


Re the point about the arrow keys, again let's keep that for building later.

One issue here is how we're defining what a 'key' is for various purposes, ie. surrounding by <kbd> tags as a keyboard-key, or as a key-combination. Currently the display function just gives a string, but we're not limited to that data format until we ultimately must output it. Also, if we style the keys (physical keyboard and combinations) in the UI, we'll likely want to think of how we structure them in a similar way.

neiljp · 2024-04-14T04:58:55Z

zulipterminal/core.py

        self.show_pop_up(help_view, "area:help")

    def show_markdown_help(self) -> None:
-        markdown_view = MarkdownHelpView(self, "Markdown Help Menu (up/down scrolls)")
+        markdown_view = MarkdownHelpView(self, "Markdown Help Menu " + SCROLL_PROMPT)


Improvements and bugfixes to the doc/linting are fine in a related PR, but this commit looks like a different overall change - useful for consistency as it is 👍

You should be able to cherry-pick it into another branch just fine.

I've not tested this yet, but you updated the formatting of other lines but not this one?

This also prompts thoughts on ensuring we're consistent with naming of the menu titles and their names in the descriptions of the related hotkeys, which I've not checked recently, and I'm not sure if it's worth checking technically, or manually.

zulipbot added the size: XL [Automatic label added by zulipbot] label Apr 11, 2024

Niloth-p force-pushed the hotkeys-doc-945 branch from 39886e1 to 5683c3a Compare April 11, 2024 18:57

zulipbot added area: hotkeys PR needs review PR requires feedback to proceed labels Apr 11, 2024

neiljp reviewed Apr 12, 2024

View reviewed changes

Niloth-p force-pushed the hotkeys-doc-945 branch from 5683c3a to 88cd99e Compare April 13, 2024 04:45

zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Apr 13, 2024

lint-hotkeys: Refine display of keys in hotkeys doc.

b682d2f

Uses the new display functions from keys.py. Applies - Capitalization of special keys. - Fix "<kbd>page</kbd> + <kbd>up</kbd>" to "<kbd>PgUp</kbd>". Fixes zulip#945.

Niloth-p force-pushed the hotkeys-doc-945 branch from 88cd99e to b682d2f Compare April 13, 2024 04:46

symbols/keys/lint-hotkeys: Append arrow symbols to arrow keys.

68fc6e3

Tests updated.

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Apr 13, 2024

Niloth-p commented Apr 13, 2024

View reviewed changes

Niloth-p added 2 commits April 13, 2024 17:48

core: Update scroll prompt in Popup headers.

e523a11

Replace the hardcoded scroll hints with the commands' display keys.

lint-hotkeys: Make alphabet keys capitalized in the hotkey docs.

3897db4

Prepends upper case alphabets with 'Shift + '.

Niloth-p force-pushed the hotkeys-doc-945 branch from c94310a to 3897db4 Compare April 13, 2024 12:21

zulipbot added the has conflicts label Apr 14, 2024

neiljp reviewed Apr 14, 2024

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Apr 14, 2024

neiljp mentioned this pull request Apr 14, 2024

Improve key rendering in documentation #945

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve hotkey rendering in documentation #1479

Improve hotkey rendering in documentation #1479

Niloth-p commented Apr 11, 2024

Niloth-p commented Apr 11, 2024

zulipbot commented Apr 11, 2024

neiljp left a comment

neiljp Apr 12, 2024

Niloth-p Apr 13, 2024 •

edited

Loading

neiljp Apr 14, 2024

neiljp Apr 12, 2024

Niloth-p left a comment •

edited

Loading

Niloth-p Apr 13, 2024

neiljp Apr 14, 2024

Niloth-p Apr 13, 2024 •

edited

Loading

Niloth-p Apr 13, 2024

zulipbot commented Apr 14, 2024

neiljp left a comment

neiljp Apr 14, 2024

neiljp Apr 14, 2024

neiljp Apr 14, 2024

Improve hotkey rendering in documentation #1479

Are you sure you want to change the base?

Improve hotkey rendering in documentation #1479

Conversation

Niloth-p commented Apr 11, 2024

What does this PR do, and why?

Outstanding aspect(s)

External discussion & connections

How did you test this?

Visual changes

Niloth-p commented Apr 11, 2024

zulipbot commented Apr 11, 2024

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niloth-p Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niloth-p left a comment • edited Loading

Choose a reason for hiding this comment

Visual Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niloth-p Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Apr 14, 2024

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niloth-p Apr 13, 2024 •

edited

Loading

Niloth-p left a comment •

edited

Loading

Niloth-p Apr 13, 2024 •

edited

Loading