-
-
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
popovers: Simplify instructions for moving topics/messages #30064
base: main
Are you sure you want to change the base?
Conversation
<p>{{t "Select a channel below or change topic name." }}</p> | ||
{{#unless (and (not from_message_actions_popover) (not only_topic_edit))}} | ||
<p>{{t "Move messages to." }}</p> | ||
{{/unless}} |
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.
How did you determine this is the correct condition to check?
Also note it's supposed to be a :
after the to
.
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.
Also should this just be an if
statement instead of an unless
? Demorgan's law seems relevant 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.
How did you determine this is the correct condition to check?
Also note it's supposed to be a
:
after theto
.
in the move_topic_to_stream.hbs
file the below text in p tag renders when both from_message_actions_popover
and only_topic_edit
are false, and when I looked it up it is basically true when "move topic" modal is open
now as suggested change I had to change the text Select a channel below or change topic name.
to Move messages to:
in the "move message" modal and had to remove from "move topic" modal. In the previous code given below it was just checking whether it is only "change topic name or not" and if not the later text was rendered in both "move message" and "move topic" modal.
so I introduced a logic when the text will not be rendered when "move topic" modal is opened, to ensure that I introduced negation of logic shown in first picture in this comment, which comes out to be
~(p or q) is equivalent to (~p and ~q)
so as the text under unless
get's rendered only when the condition under unless
is false, the message Move messages to:
does not render when both of them are true, that is when "move topic" modal is opened, but it could happen that in this case the text also might get rendered when only_edit_topic
is true but since this condition is already checked before coming to this line this does not get rendered.
while if
can be used instead of unless
but then the condition needs to be changed. Please let me know If I need to change the condition. For now I am fixing the little change and updating the PR.
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 this condition can just be simplified to given the only condition to show this text is when the user opens "Move messages" modal and it is not a topic only edit -
{{else if from_message_actions_popover}}
<p>{{t "Move messages to:" }}</p>
{{/if}}
@iks1 Please update the screenshots in the PR description, and post a comment when ready for another round of review. |
Done, Please look at it Once, |
@sahil839 are you up for reviewing this one? I haven't tested. |
@alya we also show "Rename topic to:" text in both the modals instead of "Select a channel below or change topic name." is user is only allowed to edit topics and not streams, so we want to keep that text as it is for both the modals? |
Yeah, I think "Rename topic to:" is fine as-is. |
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.
Posted one comment. Also, a couple of minor fixes in commit message would be good - the line-wrapping of point marked as "2)" can be changed to match with the above lines and the "Fixes .." line should end with a period.
<p>{{t "Select a channel below or change topic name." }}</p> | ||
{{#unless (and (not from_message_actions_popover) (not only_topic_edit))}} | ||
<p>{{t "Move messages to." }}</p> | ||
{{/unless}} |
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 this condition can just be simplified to given the only condition to show this text is when the user opens "Move messages" modal and it is not a topic only edit -
{{else if from_message_actions_popover}}
<p>{{t "Move messages to:" }}</p>
{{/if}}
Modified the `Select a channel below or change topic name.` instruction in `move topics/move messages` modal. It's not necessary and can be confusing when user does not have permissions to change the channel and topic name. 1) On the `Move topic` modal, dropped the line 2) On the `Move messages` modal, replaced the line with `Move messages to:` Fixes zulip#30055.
Made the suggested changes. Please review it. |
Looks good to me. @alya you can have a look. One thing to point out is that if in "Move messages" modal, we never show "Rename topic to :" even when user is only allowed to change topic. We always show "Move messages to:", (and before this PR we always showed "Select a channel below or change topic name.". So, this behavior is not introduced by this PR, but just mentioning in case we want to change it. |
Changed the
Select a channel below or change topic name.
instruction inmove topics/move messages
modal.Move topic
modal, dropped the lineMove messages
modal, replaced the line withMove messages to:
Fixes #30055
Screenshots and screen captures:
-- Instructions on
Move messages/topics
modal after and before changes --before changes
*after changes *
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: