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.

Sign up for GitHub

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

Jump to bottom

stream_settings: Fix stream row switching behavior when adding subscribers. #29839

Merged
merged 1 commit into from Apr 30, 2024
Merged

stream_settings: Fix stream row switching behavior when adding subscribers. #29839

merged 1 commit into from Apr 30, 2024
+38 −0

Conversation

PieterCK
Copy link
Collaborator

@PieterCK PieterCK commented Apr 25, 2024 •
edited

📑 Overview

The root cause of the bug is that the switch_rows(event) function in stream_settings_ui.js does not consider whether the user is currently editing the add_subscriber_pill input in the right panel of the stream settings menu for the key up/down event to trigger the stream row switching process.

Adjusted the switch_rows(event) behavior by adding a condition to prevent switching stream row when the user in the subscriber tab on stream settings menu (hash_parser.is_on_stream_subscribers()) and is focused on the add_subscribers_pill input

Fixes #29690
CZO

Screenshots and screen captures:

Screencast.from.25-04-24.17.46.54.webm
Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@PieterCK PieterCK force-pushed the issue-29690-add-subscriber-stream-settings-bug branch from 53ebe5b to c625852 Compare April 25, 2024 11:06
@PieterCK PieterCK mentioned this pull request Apr 25, 2024
Closed
@PieterCK
Copy link
Collaborator Author

PieterCK commented Apr 25, 2024 •
edited

Info: still missing some test and proper linting, but decided to up this PR to get feedback early

@PieterCK PieterCK force-pushed the issue-29690-add-subscriber-stream-settings-bug branch from c625852 to 4a654d9 Compare April 26, 2024 03:32
@zulipbot zulipbot added size: M and removed size: S labels Apr 26, 2024
@PieterCK
Copy link
Collaborator Author

PieterCK commented Apr 26, 2024 •
edited

update: added a new test and fixed linting

@PieterCK PieterCK force-pushed the issue-29690-add-subscriber-stream-settings-bug branch from 4a654d9 to 59119f4 Compare April 26, 2024 11:40
@alya
Copy link
Contributor

alya commented Apr 27, 2024

Thanks! @sahil839 are you up for reviewing this one? I haven't tested.

@alya alya added the maintainer review PR is ready for review by Zulip maintainers. label Apr 27, 2024
sahil839
sahil839 reviewed Apr 29, 2024
View reviewed changes
Copy link
Collaborator

@sahil839 sahil839 left a comment

Choose a reason for hiding this comment

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

Posted a couple of comments.

web/src/stream_settings_ui.js Outdated Show resolved Hide resolved
web/src/stream_settings_ui.js Outdated Show resolved Hide resolved
web/src/hash_parser.ts Outdated Show resolved Hide resolved
@PieterCK PieterCK force-pushed the issue-29690-add-subscriber-stream-settings-bug branch from 59119f4 to bfda9fa Compare April 29, 2024 07:52
@PieterCK
Copy link
Collaborator Author

@sahil839 Thanks for reviewing. I've addressed your comments for the PR and have tested it again. Let me know if there are any other changes you'd like to suggest 👍

@sahil839
Copy link
Collaborator

Just a small point - "Fixes .." line in commit message should end with a period. Looks good otherwise.

@timabbott ready for your review.

@sahil839 sahil839 added integration review Added by maintainers when a PR may be ready for integration. and removed maintainer review PR is ready for review by Zulip maintainers. labels Apr 29, 2024
@PieterCK PieterCK force-pushed the issue-29690-add-subscriber-stream-settings-bug branch from bfda9fa to b653eea Compare April 29, 2024 09:12
timabbott
timabbott reviewed Apr 29, 2024
View reviewed changes
web/tests/hash_util.test.js

window.location.hash = "#some/random/place/subscribers";
assert.equal(hash_parser.is_subscribers_section_opened_for_stream(), false);
});
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We should add a test for something more like the empty string.

@PieterCK @timabbott
stream_settings: Fix stream row switching behavior.
02a40f3 
Add a rule to the switch_rows(event) function to avoid
switching stream row when pressing up/down key by checking
the current URL hash and the add_subscribers_pill focus state.
Add a utility function to hash_parser to help implement new
switch_rows behavior mentioned above.

Fixes zulip#29690.
@timabbott timabbott force-pushed the issue-29690-add-subscriber-stream-settings-bug branch from b653eea to 02a40f3 Compare April 30, 2024 01:34
@timabbott timabbott enabled auto-merge (rebase) April 30, 2024 01:34
@timabbott
Copy link
Sponsor Member

Made the small test tweak noted above and merged, thanks @PieterCK!

@timabbott timabbott merged commit 2457a8b into zulip:main Apr 30, 2024
7 checks passed
@PieterCK PieterCK deleted the issue-29690-add-subscriber-stream-settings-bug branch April 30, 2024 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timabbott timabbott timabbott left review comments

@sahil839 sahil839 sahil839 left review comments

Assignees
No one assigned
Labels
integration review Added by maintainers when a PR may be ready for integration. size: M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable up/down keyboard navigation when adding subscribers via stream settings.
5 participants
@PieterCK @alya @sahil839 @timabbott @zulipbot