-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
topics: Convert topic links to permalinks. #30114
base: main
Are you sure you want to change the base?
Conversation
Chat threads in |
Updated narrow
|
a6b737a
to
df4829a
Compare
|
||
narrow.remove(with_term) | ||
|
||
return narrow |
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 think we will want this function to also return at least a boolean
for whether the narrow was in fact modified from its original state; I think there's a good chance we'll want to have the API response do something to reflect whether that in fact occurred.
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.
we can decide it in the client side whether the narrow has been updated or not. I updated it to use the client side implementation.
web/src/narrow.js
Outdated
id_info.final_select_id = min_defined(id_info.target_id, unread_info.msg_id); | ||
id_info.final_select_id = filter.has_operator("with") | ||
? unread_info.msg_id | ||
: min_defined(id_info.target_id, unread_info.msg_id); |
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.
Why is this the right special case?
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.
well, in case of this with
operator, we populate the target_id
with the operand. The final_select_id
in our case should always be the first unread if found locally (else undefined, so that we can fetch it from server). It is however possible that out target_id
(basically the with
operand) might be <
first unread msg_id
theroetically
web/src/narrow.js
Outdated
@@ -94,6 +94,56 @@ export function save_narrow(terms, trigger) { | |||
changehash(new_hash, trigger); | |||
} | |||
|
|||
function change_view_for_narrow_containing_with_operator(message_data, opts) { |
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'm not sure I see anywhere where this logic would avoid doing a bunch of work in the very common case that the target message has never moved. Possibly the message_fetch
API response suggested elsewhere would be helpful to you here for doing this check well.
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 added the logic so that it would avoid the work in case it is in the same narrow.
web/tests/filter.test.js
Outdated
{operator: "with", operand: 17}, | ||
]; | ||
filter = new Filter(terms); | ||
assert.ok(filter.can_bucket_by("channel", "with")); |
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.
Is it correct to say we can bucket by those operators? We don't actually know which messages are in the current conversation... We might need to be careful here.
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.
Can you comment on how you addressed this feedback detail? Generally it's best practice for every thread to include explanation of how the feedback was resolved.
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.
So in the current version of PR, I only use can_bucket_by("channel", "topic", "with")
in the PR.
Since the with
operator is not available in the search suggestion, and is only through either topic-mentions or left_sidebar, I guess it is certain that operations involving with
only contain the above operands.
This commit extracts the method to create and update message lists in the narrow. This is particularly useful in cases when we need to update the narrow and view with our own message lists. This is a preparatory commit to zulip#30114, since it involves creating a new message list from the messages fetched, which may be of a different narrow from that of the initial filter, and updating the view and narrow with it.
This commit extracts the method to create and update message lists in the narrow. This is particularly useful in cases when we need to update the narrow and view with our own message lists. This is a preparatory commit to zulip#30114, since it involves creating a new message list from the messages fetched, which may be of a different narrow from that of the initial filter, and updating the view and narrow with it.
This commit extracts the method to create and update message lists in the narrow. This is particularly useful in cases when we need to update the narrow and view with our own message lists. This is a preparatory commit to #30114, since it involves creating a new message list from the messages fetched, which may be of a different narrow from that of the initial filter, and updating the view and narrow with it.
web/src/narrow.js
Outdated
// raw_terms with both `dm` and with operators are redundant, since dms | ||
// don't have topics. Hence, we remove the `with` operator. | ||
if (raw_terms.some((term) => term.operator === "dm")) { | ||
raw_terms = raw_terms.filter((term) => term.operator !== "with"); | ||
} |
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 assume private messages containing with
operators would be redundant since they can't be moved?
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 think that's correct, it should be a noop on DMs for now.
But we may change that behavior. I'll link to #20501 so that there's a reference on that issue of this detail. I'm not convinced much more effort is required.
addressed the above reviews. |
43535b5
to
95c4e72
Compare
7bf2538
to
28d97c2
Compare
Updates:
|
This commit adds a new narrow operator -- "with" which expects a string operand of message_id, and returns all the messages in the same channel and topic of the operand, with the first unread message of the topic as anchor. This is done as an effort to provide permalinks for topics in zulip#21505.
This commit converts the links generated by the markdown of the "#-mention" of topics to permalinks -- the links containing the "with" narrow operator, the operand being the last message of the channel and topic of the mention. Partly Fixes zulip#21505
This commit updates the topic links obtained from clicking the topics and those obtained from "Copy link to topic" to use the new topic permalinks. Fixes zulip#21505
Commit 1: adds a new narrow operator -- "with" which expects a string operand of message_id, and returns all the messages in the same channel and topic of the operand, with the first unread message of the topic as anchor.
Commit 2: Converts #-mention of topics to permalinks.
Commit 3: Converts left sidebar topic links and
Copy link to topic
to permalinks.Fixes: #21505
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: