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

Create new help categories related to opening compose box #1520

Merged
merged 2 commits into from
Jul 7, 2024

Conversation

Niloth-p
Copy link
Collaborator

What does this PR do, and why?

Groups key bindings from message actions into the new categories:

  • Compose: New Message
  • Compose: Replies

And in the 2nd commit, suggests renaming the "Composing a message" category to "Writing a message" category, to improve clarity, in contrast with the newly added categories.

External discussion & connections

  • Discussed in #zulip-terminal in Re-Categorization of Hotkeys
  • Fully fixes #
  • Partially fixes issue #
  • Builds upon previous unmerged work in PR #
  • Is a follow-up to work in PR #
  • Requires merge of PR #
  • Merge will enable work on Introduce help context #1484

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual Changes

image

Copy link
Member

@rsashank rsashank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks for making this PR!

Copy link

@zormit zormit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice, thank you. I'm looking forward to having a reduced list of "message actions" (which was too long).

@zormit zormit added PR needs review PR requires feedback to proceed and removed PR needs mentor review labels Jun 26, 2024
@Niloth-p Niloth-p force-pushed the category/1520-open-compose/pr branch from a2cc3e5 to dd94301 Compare June 27, 2024 02:47
@Niloth-p
Copy link
Collaborator Author

Rebased, resolving merge conflicts and updating INSERT_NEW_LINE's category along with the rest in the 2nd commit.

@neiljp
Copy link
Collaborator

neiljp commented Jun 28, 2024

@Niloth-p Thanks for sticking with these improvements - I agree they are definitely helping 👍

Following on from my summary in #1524, my main concern here is that the categories are small (3 or 4 entries), plus the draft option doesn't entirely fit as a 'new message' (which would make it 2, and find somewhere to put the draft entry?). While it potentially makes it easier to find some entries when there are very many categories, the vertical spacing starts to take over. Do you agree?

I think we'll still gain from splitting these commands out into one section, and the renaming of the other section certainly remains worthwhile too.

Having just merged the GO_BACK splitting PR (#1514), it also prompts me to think whether the exiting of compose could be included here (too), as a 'Starting/Exiting compose' sort of category. However, that introduces multi-category commands again, and could make naming the single category more challenging - this isn't as important as getting the splitting and renaming done and merged.

@neiljp 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 Jun 28, 2024
Regroup key bindings that were previously categorized under
message actions and the general category.

Hotkeys document regenerated.
@Niloth-p Niloth-p force-pushed the category/1520-open-compose/pr branch from dd94301 to b4f977f Compare July 2, 2024 10:07
The category name in the code: 'msg_compose' -> 'compose_box'
The user visible name: 'Composing a message' -> 'Writing a message'

This is to emphasize its difference from the other newly created
category for actions that open the compose box, to both users and
developers.

Hotkeys document regenerated.
@Niloth-p Niloth-p force-pushed the category/1520-open-compose/pr branch from b4f977f to 5b6f02e Compare July 2, 2024 10:15
@Niloth-p
Copy link
Collaborator Author

Niloth-p commented Jul 2, 2024

Updates:

  • Merged the 2 new categories.
  • Rebased onto main, and updated the recently newly added EXIT_COMPOSE.

Things to check:

  • The naming of the newly formed category - both internally and externally
  • Is the ordering of help keys in the new category satisfactory?

@Niloth-p Niloth-p added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 2, 2024
@Niloth-p
Copy link
Collaborator Author

Niloth-p commented Jul 2, 2024

While it potentially makes it easier to find some entries when there are very many categories, the vertical spacing starts to take over. Do you agree?

Regarding my opinion,
The spacing would definitely dominate in smaller screens. 3 is quite a small category indeed.

But, I like smaller help categories in general for any application, but that's probably because I try to memorize them (and the related commands) instead of just referring to a single command.
So, tighter categories help in that aspect, instead of me having to break them down into further tiny categories in my own mind, it's already done for me. So, the tightness/fit of entries into categories would be more preferred over the size of categories.
But, that's probably me, I have no idea what users majorly prefer.

@zormit
Copy link

zormit commented Jul 4, 2024

I think small categories can be nice as long as the titles are really really clear.

My reason would be a bit different than yours: If I'm looking for a specific command I can just skim the headlines and hopefully quickly find the right category. In that category it then should also be quite fast to identify the command I'd be looking for, because there are not that many present.

@neiljp
Copy link
Collaborator

neiljp commented Jul 7, 2024

@Niloth-p Thanks for this and your slight update 👍 It's definitely nuanced between the two but let's move on and get this merged for now, and we can always return to examine the category splits later, particularly if we have the section jump keys :)

@neiljp neiljp merged commit 338e6b5 into zulip:main Jul 7, 2024
21 checks passed
@neiljp neiljp added this to the Next Release milestone Jul 7, 2024
@neiljp neiljp removed the PR needs review PR requires feedback to proceed label Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: popup: help size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants