Skip to content

Conversation

neiljp
Copy link
Collaborator

@neiljp neiljp commented Jan 25, 2021

This refactors the view update (originally by @sumanthvrao) on pushing newer topics to the top of the topic list, making the logic cleaner.

The final commit fixes #661.

NOTES:

  • The tests still need updating, which I'm leaving as pending until I'm more sure this is the right approach.
  • I'm considering a further refactor or adjustment of the last commit to deduplicate the loop or use a lookup dict.

Feedback welcome

This clarifies the intent of the function, the filtering of the topics
that occurs from the search box, compared to simply 'updating' it, which
is less clear and currently very similar to the adjacent
update_topics_list.

Tests amended.
Use two intermediate variables to improve code readability.
However, be sure to do the operation on the un-filtered (non-search)
list, for when search is exited.

Fixes zulip#661.
@neiljp neiljp added bug Something isn't working feedback wanted labels Jan 25, 2021
@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Jan 25, 2021
@neiljp neiljp added this to the Release after next milestone Jan 28, 2021
Base automatically changed from master to main January 30, 2021 20:31
@zulipbot
Copy link
Member

Heads up @neiljp, we just merged some commits that conflict with the changes your 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feedback wanted has conflicts size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating topic list while in topic search adds topic even if it doesn't fit the search
2 participants