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

Improve spoiler titles #1033

Merged
merged 5 commits into from
Jan 15, 2024

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Jan 9, 2024

Pull Request Description

Important

The following description has changed slightly. Please read all the comments in the thread.

This PR has a couple small tweaks for spoilers.

  1. Added a Material as the root widget of CommonMarkdownBody. This should ensure that InkWells always work (which they previously didn't in places like community sidebar).
  2. I adjusted the horizontal translation of the heading. I realized this was done to make the start of the text align with the rest of the body, but the end result was that the corner radius of the InkWell was cut off, so only the right side was rounded. However, I also didn't like the text being misaligned, so I removed the left padding. This means the text now bumps up against the InkWell, but I think it looks better than having square corners.
  3. Finally, I scaled down the arrow to better align single-line headings.

Review without whitespace.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

After

spoiler_title_after.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@hjiangsu
Copy link
Member

I am leaving this as a draft, since I realized that the transform works fine in some places (like post body) but not others (like community sidebar) so I am rethinking whether there's still a way to utilize transform so that we can keep the left padding.

Ahh yeah, I initially had InkWells for the spoiler markdown as you indicated in the example, but I bumped into the same issue! I do want the text the be aligned with the rest of the text otherwise I feel like it looks out of place. I didn't have a good solution at that time so I left the InkWell out 😅

@micahmo
Copy link
Member Author

micahmo commented Jan 10, 2024

It looks like this is a bit of a catch-22. The InkWells don't work in the sidebars because there's no Material in the tree. But as soon as I add a Material, the InkWells get cut off due to the horizontal translation. Still working on this one. 😊

@micahmo
Copy link
Member Author

micahmo commented Jan 11, 2024

Quick update! I tried everything I could, but I could not prevent the clipping that I was seeing from the horizontal transformation within the sidebars. I even tried setting Clip.none to every applicable widget all the way up the tree, but it didn't help.

My last solution is support conditional transformation. It's a bit clunky, but now things look good both in regular views and in the sidebars. The only exception is that in the sidebars, there is no left padding between the spoiler title and the edge of the InkWell. But at least the InkWell is now visible and not cut off! And also the text remains aligned with the rest of the body in every case.

Here is the latest demo, let me know what you think..

qemu-system-x86_64_xl0oLBGqBh.mp4

@micahmo micahmo marked this pull request as ready for review January 11, 2024 18:53
@hjiangsu
Copy link
Member

I think this works! Thanks for doing this, and figuring this out!

@hjiangsu hjiangsu merged commit 0f97940 into thunder-app:develop Jan 15, 2024
1 check passed
@micahmo micahmo deleted the feature/improve-spoiler-title branch January 15, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants