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.

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

Fix bad rendering of new topics when topics list was open. #1062

Merged
merged 9 commits into from
Jul 28, 2021

Conversation

zee-bit
Copy link
Member

@zee-bit zee-bit commented Jul 2, 2021

This PR fixes the bug that made incoming new topics rendered badly when the topic list was open due to incorrect parameters being passed to TopicButton. This PR also migrates TopicButton to accept only named parameters so that there's no scope of bug and connascence between parts of the program is reduced.

The discussion regarding this bug can be found in #zulip-terminal>New-topic topic-list bug

@zulipbot zulipbot added the size: S [Automatic label added by zulipbot] label Jul 2, 2021
@zee-bit zee-bit force-pushed the fix-new-topic-topic-list-bug branch from e1d3c9c to 5ee3136 Compare July 2, 2021 21:21
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Jul 2, 2021
@zee-bit zee-bit force-pushed the fix-new-topic-topic-list-bug branch from 5ee3136 to dad59c1 Compare July 2, 2021 21:59
@zee-bit zee-bit added area: UI General user interface update PR needs review PR requires feedback to proceed labels Jul 2, 2021
@neiljp
Copy link
Collaborator

neiljp commented Jul 4, 2021

@zee-bit Thanks again for finding and fixing the bug 👍 As per the stream, I've merged the first commit (after blackening), and I suggest we can include removal of the default count value in the remaining commits.
(merged commit: 4e41853)

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jul 4, 2021
@zee-bit zee-bit force-pushed the fix-new-topic-topic-list-bug branch 2 times, most recently from ccf558a to d47ec04 Compare July 10, 2021 21:02
@zee-bit zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 10, 2021
@zee-bit zee-bit force-pushed the fix-new-topic-topic-list-bug branch from d47ec04 to f8e5113 Compare July 14, 2021 19:57
This commit adds the "*" method of passing required parameters using
keyword arguments to TopicButton. This ensures that we pass the correct
parameters to our class and don't miss any, thus preventing bugs and
reducing connascence between parts of the program.

We also remove the default value of count from the parameters, because
we are explicitly calling it with some value, and this will help avoid
any potential issues in the future.
This commit adds the "*" method of passing required parameters using
keyword arguments to HomeButton. This ensures that we pass the correct
parameters to our class and don't miss any, thus preventing bugs and
reducing connascence between parts of the program.

We also remove the default value of count from the parameters, because
we are explicitly calling it with some value, and this will help avoid
any potential issues in the future.

Tests amended.
This commit adds the "*" method of passing required parameters using
keyword arguments to TopicButton. This ensures that we pass the correct
parameters to our class and don't miss any, thus preventing bugs and
reducing connascence between parts of the program.

We also remove the default value of count from the parameters, because
we are explicitly calling it with some value, and this will help avoid
any potential issues in the future.

Tests amended.
This commit adds the "*" method of passing required parameters using
keyword arguments to MentionedButton. This ensures that we pass the
correct parameters to our class and don't miss any, thus preventing
bugs and reducing connascence between parts of the program.

We also remove the default value of count from the parameters, because
we are explicitly calling it with some value, and this will help avoid
any potential issues in the future.
This commit adds the "*" method of passing required parameters using
keyword arguments to StarredButton. This ensures that we pass the correct
parameters to our class and don't miss any, thus preventing bugs and
reducing connascence between parts of the program.

We also remove the default value of count from the parameters, because
we are explicitly calling it with some value, and this will help avoid
any potential issues in the future.

Tests amended.
This commit adds the "*" method of passing required parameters using
keyword arguments to StreamButton. This ensures that we pass the correct
parameters to our class and don't miss any, thus preventing bugs and
reducing connascence between parts of the program.

We also remove the default value of count from the parameters, because
we are explicitly calling it with some value, and this will help avoid
any potential issues in the future.

Tests amended.
This commit adds the "*" method of passing required parameters using
keyword arguments to UserButton. This ensures that we pass the correct
parameters to our class and don't miss any, thus preventing bugs and
reducing connascence between parts of the program.

We also remove the default value of count from the parameters, because
we are explicitly calling it with some value, and this will help avoid
any potential issues in the future.

Tests amended.
This commit adds the "*" method of passing required parameters using
keyword arguments to MessageLinkButton.

Tests amended.
This commit adds the "*" method of passing required parameters using
keyword arguments to EditModeButton.
@neiljp neiljp force-pushed the fix-new-topic-topic-list-bug branch from f8e5113 to 3f3c82c Compare July 28, 2021 23:39
@neiljp
Copy link
Collaborator

neiljp commented Jul 28, 2021

@zee-bit Thanks for the update for the counts 👍 I just pushed a minor commit text adjustment to remove width mention (that was removed already elsewhere?) and add a missing *.

I'm waiting for tests to pass and then will merge 🎉

@neiljp neiljp added PR ready to be merged PR has been reviewed & is ready to be merged area: refactoring bug Something isn't working and removed PR needs review PR requires feedback to proceed labels Jul 28, 2021
@zulipbot
Copy link
Member

Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

@neiljp neiljp merged commit ab4a088 into zulip:main Jul 28, 2021
@neiljp neiljp added this to the Next Release milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: refactoring area: UI General user interface update bug Something isn't working PR ready to be merged PR has been reviewed & is ready to be merged size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants