Skip to content

Break down the GO_BACK command into separate commands #1514

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

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

Niloth-p
Copy link
Collaborator

@Niloth-p Niloth-p commented Jun 6, 2024

What does this PR do, and why?

Breaks GO_BACK command into the following commands:

  • CLEAR_SEARCH (clears search box and search list of current panel)
  • EXIT_COMPOSE (clears compose box state and closes it)
  • EXIT_POPUP (closes the currently open popup)

This would enable using more specific help texts, categories and context.
And clarifies the purposes of the key better for users and developers.

I've kept these as separate commits for the sake of readability. Introducing each command in its own commit, then finally removing GO_BACK command.
I could break #1497 into commits similar to this one for readability, if necessary.
I've kept this as a separate PR as it's an independent change, even though the motivation is shared.

External discussion & connections

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

@zulipbot
Copy link
Member

zulipbot commented Jun 6, 2024

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

@Niloth-p Niloth-p force-pushed the category/1514-break-esc/pr branch from bbd7b2e to d449a0d Compare June 7, 2024 01:24
Comment on lines +251 to +271
'CLEAR_SEARCH': {
'keys': ['esc'],
'help_text': 'Clear search in current panel',
'key_category': 'searching',
},
Copy link
Member

Choose a reason for hiding this comment

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

Since we're aiming at contextual help messages, should we be using the word panel? It feels more like code terminology and might not be useful as a help message.

Copy link
Collaborator Author

@Niloth-p Niloth-p Jun 11, 2024

Choose a reason for hiding this comment

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

Hmm.
Found mentions of 'panel' in the user facing documentation.
What other word do you think would fit better in this context?
Edit: As discussed, it's hard to come up with suitable alternatives, so I've left it as 'panel'.

@rsashank
Copy link
Member

rsashank commented Jun 7, 2024

LGTM! The intention seems clear since we're aiming for contextual help.

Just a note: Either this PR or #1442 will need to handle conflicts in a rebase, depending on which gets merged first.

@Niloth-p Niloth-p 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 PR needs mentor review labels Jun 10, 2024
@Niloth-p Niloth-p force-pushed the category/1514-break-esc/pr branch from d449a0d to 02f3c98 Compare June 12, 2024 04:14
@Niloth-p Niloth-p added PR needs mentor review and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 12, 2024
@Niloth-p Niloth-p requested a review from zormit June 12, 2024 04:32
@Niloth-p Niloth-p force-pushed the category/1514-break-esc/pr branch from 02f3c98 to 0605a95 Compare June 24, 2024 06:42
@Niloth-p Niloth-p force-pushed the category/1514-break-esc/pr branch from 0605a95 to d0a086c Compare June 24, 2024 06:53
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 pretty nicely structure and looks good to me. Thank you!

@zormit zormit added PR needs review PR requires feedback to proceed and removed PR needs mentor review labels Jun 26, 2024
Niloth-p added 3 commits June 27, 2024 23:37
Tests updated.
Hotkeys document regenerated.
Added an exclusion in lint-hotkeys for `Esc` as both EXIT_POPUP and
ALL_MESSAGES commands belong to the help category 'Navigation'.

Tests updated.
Hotkeys document regenerated.
@neiljp neiljp force-pushed the category/1514-break-esc/pr branch from d0a086c to 97e8e20 Compare June 28, 2024 07:03
@neiljp neiljp added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR needs review PR requires feedback to proceed labels Jun 28, 2024
@neiljp
Copy link
Collaborator

neiljp commented Jun 28, 2024

@Niloth-p I agree with @zormit - this is clear and easy to read 👍

The changes I pushed back with were only structural/text (the code overall is identical):

  • squash the last commit with the smaller of the two later commits, to avoid a state where we have a menu entry that does nothing
  • adjust some commit text (these change the UI, so aren't strictly refactors
  • added some file names in the commit titles

@neiljp neiljp merged commit d46b6d8 into zulip:main Jun 28, 2024
@neiljp neiljp added this to the Next Release milestone Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: popup: help area: refactoring PR ready to be merged PR has been reviewed & is ready to be merged size: XL [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants