Skip to content

content [nfc]: Use Divider instead of DecoratedBox. #836

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
wants to merge 1 commit into from

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Jul 25, 2024

This should offer the same appearance for the spoiler divider despite being shorter.

Discovered while working on #823.

@PIG208
Copy link
Member Author

PIG208 commented Jul 25, 2024

Before After
Screenshot from 2024-07-24 20-04-26 Screenshot from 2024-07-24 20-04-16
side by side

image

@PIG208 PIG208 marked this pull request as ready for review July 25, 2024 00:08
@PIG208 PIG208 requested a review from chrisbobbe July 25, 2024 20:38
@PIG208 PIG208 added maintainer review PR ready for review by Zulip maintainers a-content Parsing and rendering Zulip HTML content, notably message contents labels Jul 25, 2024
@chrisbobbe
Copy link
Collaborator

When I open the "Before" and "After" images in separate Chrome tabs, then switch between the tabs, I do see a difference in the appearance. I see more whitespace above the divider in the "After" image.

@PIG208
Copy link
Member Author

PIG208 commented Jul 25, 2024

Made a side-by-side version after zooming in on browser tabs:

image

@PIG208
Copy link
Member Author

PIG208 commented Jul 25, 2024

Oh I do find the difference visible when switching between tabs now. Let me investigate.

This should offer the same appearence for the spoiler divider
despite being shorter.

Signed-off-by: Zixuan James Li <[email protected]>
@PIG208
Copy link
Member Author

PIG208 commented Jul 25, 2024

Changed the height to 0. It should look identical to the old version now. An easy way to check in dev is through hot reloading with and without the commit.

Before After
image Screenshot from 2024-07-25 16-48-31

@chrisbobbe
Copy link
Collaborator

Sure, seems fine to me. Over to Greg for his review.

@chrisbobbe chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jul 25, 2024
@PIG208 PIG208 requested a review from gnprice July 29, 2024 19:36
@gnprice
Copy link
Member

gnprice commented Jul 29, 2024

I think this refactor isn't helpful. Divider is a Material widget — its main job is that it pulls in a bunch of defaults from Material Design.

This overrides three of the five defaults (color, height, thickness — the thickness defaults to 1 but is effectively forced to 0 by the zero height); for the other two, we want indent of zero not because that's the Material 3 default but because it's our design. We often use Material widgets for things like buttons and app navigation, but I think for the message content we generally have a pretty firm idea of what we want the design to look like and don't want it to depend on the choices in Material.

@PIG208
Copy link
Member Author

PIG208 commented Jul 29, 2024

Thanks for the explanation. Closing the PR.

@PIG208 PIG208 closed this Jul 29, 2024
@PIG208 PIG208 deleted the divider branch August 21, 2024 20:21
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 integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants