zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

Make editor actions a help category

Open Niloth-p opened this issue 1 year ago • 1 comments

What does this PR do, and why?

Now that all editors support readline shortcuts after the merge of #1492, all the readline shortcuts have been grouped into their own category.

External discussion & connections

  • [x] Discussed in #zulip-terminal in Re-categorization of hotkeys and Readline shortcuts in search`
  • [ ] Fully fixes #
  • [ ] Partially fixes issue #
  • [ ] Builds upon previous unmerged work in PR #
  • [x] Is a follow-up to work in PR #1492
  • [ ] Requires merge of PR #
  • [ ] Merge will enable work on #

How did you test this?

  • [ ] Manually - Behavioral changes
  • [x] 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

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

Visual changes

image

Niloth-p avatar May 09 '24 15:05 Niloth-p

Visual Changes

Before re-ordering: Screenshot from 2024-05-12 17-32-03 After re-ordering: Screenshot from 2024-05-12 17-27-43

Niloth-p avatar May 12 '24 17:05 Niloth-p

Updated Help Menu

Screenshot from 2024-05-14 02-03-42 Screenshot from 2024-05-14 02-04-03

Updated Hotkeys.md

Screenshot from 2024-05-14 02-04-27

Niloth-p avatar May 13 '24 20:05 Niloth-p

@zormit I started the original reviewing here, but would welcome any feedback you have :)

neiljp avatar May 17 '24 05:05 neiljp

@zormit The wording for the final commit was not particularly discussed anywhere beyond the feedback here. So, they're not yet decided.

The tagging of commands has been done as part of #1498 after Neil's recommendation in the feedback for this PR.

The suggestion of programmatic synchronization was made recently, after #1498 was opened, so has not been attempted yet.

Niloth-p avatar May 20 '24 18:05 Niloth-p

LGTM. I've caught up with the previous comments left, and everything looks in order to me. Removing the buddy review tag.

I apologize for the delay in buddy reviews; I was unavailable due to exams when the buddy review tag was added. I'll be quicker with reviews now on.

rsashank avatar May 28 '24 09:05 rsashank

So from my point of view, to finalize this PR you'll need to

  • [ ] make sure the wording is final (this requires a review by/discussion with @neiljp )
  • [ ] update the last commit message (I'm imagining a description how you came up with the changes and why)

(edit: i'll remove "needs mentor review" tag as well, but please let me know if I can help getting this over the line or just re-add it, if needed)

zormit avatar May 29 '24 15:05 zormit

Just added a commit description to the last commit, as per feedback. @zormit This good?

Niloth-p avatar May 31 '24 18:05 Niloth-p

@Niloth-p I just pushed the textual corrections I mentioned, and again after rebasing against a PR I just pushed to allow long URLs in the commit text body, and am merging now - thanks again! :tada:

Please do try a git range-diff against your local branch to see the specific changes; we can discuss that in the project stream or private stream, if that would be helpful more generally.

neiljp avatar Jun 01 '24 02:06 neiljp