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

OLED color theme fixes, UI alignment fixes, community insert fix #1347

Merged
merged 5 commits into from
May 5, 2024

Conversation

CTalvio
Copy link
Collaborator

@CTalvio CTalvio commented May 2, 2024

Pull Request Description

This is a collection of tiny coloring and UI changes

Issue Being Fixed

The OLED theme has several places where things go awry if it is used together with material you. This looks to change theming so that material you colors are applied correctly.

I also did some small alignment fixes, and removed an extraneous divider that only shows up on ones own profile.

Maybe could have been a separate PR, but lastly this changes community insert action to simply pasting the ! format plaintext name of the community, as this is clickable in most clients without taking users off-instance. The insert also erroneously used an @ instead of an !.

Screenshots / Recordings

image
image
image
image
image

Checklist

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

@micahmo
Copy link
Member

micahmo commented May 3, 2024

image

This should address one of @codenyte's issues here.

@codenyte
Copy link

codenyte commented May 3, 2024

Thanks so much for this PR, I was about to open an issue for the various bugs in the black theme. I feel like the Pitch black theme isn't tested as much as the light and regular dark theme.

@CTalvio
Copy link
Collaborator Author

CTalvio commented May 3, 2024

@codenyte I'll likely be dailying the oled theme when this gets fixed. I want to use it but the various bugs in it bothered me too much.

Hopefully with my eyes on it it'll be better going forward.

@codenyte
Copy link

codenyte commented May 3, 2024

I've been using the theme for months (the first thing I do after downloading an app is checking whether there's a OLED/Pitch black mode) and although these issues infinitely bothered me, I never got around to file a bug report. Very happy that it's getting fixed. I'll also keep an eye out for this and report any issues after this gets merged.

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

lib/comment/view/create_comment_page.dart Show resolved Hide resolved
@codenyte
Copy link

codenyte commented May 3, 2024

I found another weird quirk of the OLED theme:

This only happens when "Hide Top Bar on Scroll" is enabled

@codenyte
Copy link

codenyte commented May 3, 2024

Also, on the settings page, the search bar right beneath the top app bar looks kinda weird. Looks like it could use some padding on the top. Or just remove the gray background of the top app bar, I think it looks pretty bad (not just on the settings page).

@CTalvio
Copy link
Collaborator Author

CTalvio commented May 3, 2024

I'm going to leave this PR unchanged to get it merged, but feel free to open an issue and list as many spots in the OLED theme that need fixing as you can find for me to address in a future PR.

@codenyte
Copy link

codenyte commented May 3, 2024

#1353

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I'll run a build with these changes to see if I notice anything off with dark mode (since that's the one I usually use)

@hjiangsu hjiangsu merged commit d909257 into thunder-app:develop May 5, 2024
1 check passed
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.

None yet

4 participants