Skip to content

compose: Error banner for deactivated DM recipient has lost its top margin #1089

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

Closed
chrisbobbe opened this issue Nov 27, 2024 · 1 comment · Fixed by #1090
Closed

compose: Error banner for deactivated DM recipient has lost its top margin #1089

chrisbobbe opened this issue Nov 27, 2024 · 1 comment · Fixed by #1090
Assignees
Labels
a-compose Compose box, autocomplete, attaching files/images a-design Visual and UX design

Comments

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Nov 27, 2024

In 3adb395 (fyi @PIG208), we regressed on the layout of the error banner shown when a DM recipient is deactivated:

Before After
image image

The top margin disappeared, and it looks buggy.

I plan to fix this in a PR that's mostly focused on proposing some prep refactors to make a cleaner starting point for #720. (If those refactors seem good, Zixuan can rebase #924 past them.)

@chrisbobbe chrisbobbe added a-compose Compose box, autocomplete, attaching files/images a-design Visual and UX design labels Nov 27, 2024
@chrisbobbe chrisbobbe self-assigned this Nov 27, 2024
@PIG208
Copy link
Member

PIG208 commented Nov 27, 2024

Thanks to sending this issue! eb9fccd (from #924) intersects with this by using the same _ErrorBanner class for the error banner here and the one that appears above the compose box.

I noticed that I didn't post a screenshot for this. Added it to #924. Also starting a discussion on the design of this.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Nov 27, 2024
This cherry-picks (with some adjustments) UI changes from Zixuan's
current revision of PR zulip#924 (for issue zulip#720):

  compose: Support error banner redesign for replacing the compose box

See the Figma:
  https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-15770&node-type=frame&t=vOSEWlXAhBa5EsAv-0

The new design is meant to be flush with the top of the compose-box
surface, so zulip#1089 (where the old banner's top margin disappeared,
looking buggy) is resolved.

The surface (but not content) of the redesigned banner seems like it
should extend through any left/right device insets to those edges of
the screen. By taking the banner as param separate from `body`,
_ComposeBoxContainer lets the banner do that -- without dropping its
responsibility for keeping the body (inputs, compose buttons, and
send button) out of the insets. The body doesn't need to consume the
insets itself and it's already complicated enough without needing
its own SafeAreas.

It also seems like the banner should pad the bottom inset when it's
meant to replace the body, so we do that and make the expectation
clear in the `errorBanner` dartdoc.

Co-authored-by: Zixuan James Li <[email protected]>
Fixes: zulip#1089
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Dec 3, 2024
This cherry-picks (with some adjustments) UI changes from Zixuan's
current revision of PR zulip#924 (for issue zulip#720):

  compose: Support error banner redesign for replacing the compose box

See the Figma:
  https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-15770&node-type=frame&t=vOSEWlXAhBa5EsAv-0

The new design is meant to be flush with the top of the compose-box
surface, so zulip#1089 (where the old banner's top margin disappeared,
looking buggy) is resolved.

The surface (but not content) of the redesigned banner seems like it
should extend through any left/right device insets to those edges of
the screen. By taking the banner as param separate from `body`,
_ComposeBoxContainer lets the banner do that -- without dropping its
responsibility for keeping the body (inputs, compose buttons, and
send button) out of the insets. The body doesn't need to consume the
insets itself and it's already complicated enough without needing
its own SafeAreas.

It also seems like the banner should pad the bottom inset when it's
meant to replace the body, so we do that and make the expectation
clear in the `errorBanner` dartdoc.

Co-authored-by: Zixuan James Li <[email protected]>
Fixes: zulip#1089
@gnprice gnprice closed this as completed in 5b3ca51 Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-compose Compose box, autocomplete, attaching files/images a-design Visual and UX design
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants