Skip to content

content/msglist: Fix vertical position of horizontal scrollbar in codeblock #738

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

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

chrisbobbe
Copy link
Collaborator

In the issue, Greg said:

#736 (comment)

Wrapping in a SliverSafeArea would mean that the actual list stays out of the bottom inset area, right? That doesn't seem like the desired behavior — I think it's expected that the messages scroll visibly through that bottom area.

But that's not what I'm seeing. There is a visible change: the blank space that you can scroll into view, under the messages, is now taller by the height of the bottom inset. (Before, the height was just 36px hard-coded.) But you can still scroll this blank space out of view, and when you do, the messages scroll visibly through that bottom area.

Before After
image image

GIF from after:

Fixes: #736

@chrisbobbe chrisbobbe added a-content Parsing and rendering Zulip HTML content, notably message contents a-msglist The message-list screen, except what's label:a-content labels Jun 17, 2024
@chrisbobbe chrisbobbe requested a review from gnprice June 17, 2024 23:13
@chrisbobbe
Copy link
Collaborator Author

Ah and of course screenshots of the bugfix itself:

Before After
image Screenshot 2024-06-17 at 4 14 07 PM

@gnprice
Copy link
Member

gnprice commented Jun 18, 2024

I see. I guess it makes sense that that's what happens once I think about it again, and particularly after looking at the implementation of SliverSafeArea — it turns out to be quite simple, just a SliverPadding and a MediaQuery.removePadding:

  Widget build(BuildContext context) {
    assert(debugCheckHasMediaQuery(context));
    final EdgeInsets padding = MediaQuery.paddingOf(context);
    return SliverPadding(
      padding: EdgeInsets.only(
        left: math.max(left ? padding.left : 0.0, minimum.left),
        top: math.max(top ? padding.top : 0.0, minimum.top),
        right: math.max(right ? padding.right : 0.0, minimum.right),
        bottom: math.max(bottom ? padding.bottom : 0.0, minimum.bottom),
      ),
      sliver: MediaQuery.removePadding(
        context: context,
        removeLeft: left,
        removeTop: top,
        removeRight: right,
        removeBottom: bottom,
        child: sliver,
      ),
    );
  }

Seems fine; merging. Thanks for the fix!

I don't love the extra space this causes below the mark-read button, but I think that's mostly that I don't love that extra space in general; this can reasonably be seen as just bringing the combined-feed narrow in line with the others in showing that extra space.

@gnprice gnprice force-pushed the pr-fix-code-block-scrollbar branch from 543c960 to 5a1ebdb Compare June 18, 2024 00:06
@gnprice gnprice merged commit 5a1ebdb into zulip:main Jun 18, 2024
1 check passed
@chrisbobbe chrisbobbe deleted the pr-fix-code-block-scrollbar branch June 18, 2024 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-content Parsing and rendering Zulip HTML content, notably message contents a-msglist The message-list screen, except what's label:a-content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

content/msglist: Horizontal scrollbar in code block sometimes has wrong vertical position
2 participants