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
hashchange: Select the new near id if it is already rendered. #29875
base: main
Are you sure you want to change the base?
Conversation
If in the same narrow, we just want to select a new id which is already rendered, we don't need to call `narrow.activate`, we can just select the new id.
// Remove elements from hash that don't effect the narrow. | ||
// TODO: Expand the regex to include more items we can ignore. | ||
hash1 = hash1.replace(/near\/\d+$/, ""); | ||
hash2 = hash2.replace(/near\/\d+$/, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be calculated using the filter.ts
module helpers -- regexs will always be inaccurate, but we can check that they have the same set of sorted operators and operands, with perhaps the exception of a near
operator in one or the other.
// Just select the message if the narrow is the same as the current narrow and | ||
// we already have the new message we want to select rendered. | ||
// TODO: Implement a filter comparison function so that we do it for any two narrows | ||
// when calling narrow.activate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree with the thought that we should be doing this in narrow.activate
if possible, so that if we add some helper click handler it still works as designed.
if (filter.has_operator("near")) { | ||
const target_id = Number.parseInt(filter.operands("near")[0], 10); | ||
if (message_lists.current.get(target_id)) { | ||
message_lists.current.select_id(target_id, {then_scroll: true}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if the narrow.activate
call had a then_select_offset
calculated, we might want to additionally scroll to that offset?
If in the same narrow, we just want to select a new id which is already rendered, we don't need to call
narrow.activate
, we can just select the new id.