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

Unify confirmation popups into popup framework #1534

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

neiljp
Copy link
Collaborator

@neiljp neiljp commented Jul 17, 2024

What does this PR do, and why?

This incrementally improves the popup code until a point where the confirmation popups can be handled in the same way as others and improved upon:

  • extract the border styling from core.py and into a new PopUpFrame class in view.py
  • extend PopUpFrame to handle title-less cases (ie. for confirmation popups)
  • simplify confirmation popups to not support locations other than center, easing integration
  • migrate ConfirmationPopUpView from an Overlay to a Frame, and other changes to enable use of show_pop_up just like other popups

Other than simplifying the code, this also results in confirmation popups acquiring a solid edge, as with regular popups.

Outstanding aspect(s)

  • Last commit needs more consideration
  • Nesting of popups into frame suggests frame could be an urwid container/decorator class
  • Confirm ability to style popup borders
  • Select appropriate popup borders for different confirmation popups?

External discussion & connections

  • Discussed in #zulip-terminal in topic
  • Fully fixes #
  • Partially fixes issue #
  • Builds upon previous unmerged work in PR #
  • Is a follow-up to work in PR #
  • Requires merge of PR #
  • Merge will enable work on #

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

Ideally the extra height would be calculated automatically in
PopUpFrame.
These popups are all now implicitly centered.

Tests updated and tidied.
Small comment added re importance of retaining self.loop.run() for
signal handler.

Tests updated.
@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!

@zulipbot zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants