improve contrast between link and image post icons

tom-james-watson commented 9 months ago

Pull Request Description

This changes the colours used for compact list view posts, that indicate whether it is an image or a link post, to be more easily distinguishable. It does this while sticking with plain derivatives of the primary colour.

Issue Being Fixed

Can't easily distinguish link and image post icons. It's hard to know which thumbnails should be clicked on to view an image, for example.

Issue Number: https://github.com/thunder-app/thunder/issues/596

Screenshots / Recordings

For before image, see https://github.com/thunder-app/thunder/issues/596.

Checklist

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

CTalvio commented 9 months ago

I'm only seeing two colors here, whats the text post color?

tom-james-watson commented 9 months ago

I hadn't seen the setting for text post indicator. Will take a look at what I can do.

micahmo commented 9 months ago

This looks awesome!

I wonder if it would work with user chips too. ☺

EDIT: Don't forget to check how it looks in dark mode as well.

CTalvio commented 9 months ago

This does not work reliably. Here is a low-contrast example.

tom-james-watson commented 9 months ago

My last commit should address that - I updated to use onPrimaryContainer for the icon that shows on primaryContainer. Will update the screenshots.

CTalvio commented 9 months ago

Please note:

The indicators currently uses the primary, secondary and tertiary accent colors. They use the container colors to ensure contrast on both light and dark mode, they previously used the normal accent colours, this did not provide adequate contrast with all themes, do not use these colors.

If you want additional separation of the three colors, and have them be based on the theme colors, you will likely need to write your own color derivator. Also keep in mind that there will eventually be a fourth for video.

tom-james-watson commented 9 months ago

I've updated the screenshots with the latest changes, including one using (I think) the example you gave.

tom-james-watson commented 9 months ago

I wonder if it would work with user chips too. ☺

What do you mean by this by the way? I assume it would be a separate issue / PR but maybe I can take a look

CTalvio commented 9 months ago

I'm not sure I like every icon being a combination of unrelated theme presets.

CTalvio commented 9 months ago

I wonder if it would work with user chips too. relaxed

What do you mean by this by the way? I assume it would be a separate issue / PR but maybe I can take a look

He's referring to the username highlights of OP, self, mod, and admin.

CTalvio commented 9 months ago

I like the approach of flipping the colors of some icons to create distinction, however.

How about this:

Media (image, and in the future, video posts): containerColor for background, onContainerColor for icon. Written media (links and text post): onContainerColor for background, containerColor for background

Edit, tho not a fan of the straight white/black background, but I have a solution.

tom-james-watson commented 9 months ago

I'm not sure I like every icon being a combination of unrelated theme presets.

Whether people like it or not I will leave it up to you maintainers, of course. Though having the different types of post be more distinguishable was obviously the intention.

Honestly if I were the one making the decisions, I would just drop the "text post indicator" option completely and then also drop the icon from link posts. That way you're left with only icons on image posts (and in the future video posts), which to me makes sense because these are the only ones where the thumbnail is actually "interactive". It would be similar to how Relay for Reddit works.

EDIT - this is talking from the perspective of someone using compact list view. I understand how the link indicator makes sense on the default feed because it shows the URL :)

CTalvio commented 9 months ago

You can disable text post thumbnails in settings.

tom-james-watson commented 9 months ago

How about this:

Media (image, and in the future, video posts): containerColor for background, onContainerColor for icon. Written media (links and text post): onContainerColor for background, containerColor for background

Do you mean primaryContainer and onPrimaryContainer? Those properties you refer to don't exist? Looks like you have a different colour completely vs the link icon though.

CTalvio commented 9 months ago

Padding(
          padding: const EdgeInsets.only(
            left: 2.5,
            top: 2.5,
          ),
          child: postViewMedia == null || postViewMedia.media.isEmpty
              ? Material(
                  borderRadius: const BorderRadius.only(
                    bottomLeft: Radius.circular(4),
                    bottomRight: Radius.circular(12),
                    topLeft: Radius.circular(12),
                    topRight: Radius.circular(4),
                  ),
                  color: Color.alphaBlend(theme.colorScheme.primaryContainer.withOpacity(0.15), theme.colorScheme.onPrimaryContainer),
                  child: Icon(size: 17, Icons.wysiwyg_rounded, color: theme.colorScheme.primaryContainer),
                )
              : postViewMedia.media.firstOrNull?.mediaType == MediaType.link
                  ? Material(
                      borderRadius: const BorderRadius.only(
                        bottomLeft: Radius.circular(4),
                        bottomRight: Radius.circular(12),
                        topLeft: Radius.circular(12),
                        topRight: Radius.circular(4),
                      ),
                      color: Color.alphaBlend(theme.colorScheme.secondaryContainer.withOpacity(0.15), theme.colorScheme.onSecondaryContainer),
                      child: Icon(
                        size: 19,
                        Icons.link_rounded,
                        color: theme.colorScheme.secondaryContainer,
                      ),
                    )
                  : Material(
                      borderRadius: const BorderRadius.only(
                        bottomLeft: Radius.circular(4),
                        bottomRight: Radius.circular(12),
                        topLeft: Radius.circular(12),
                        topRight: Radius.circular(4),
                      ),
                      color: theme.colorScheme.primaryContainer,
                      child: Icon(size: 17, Icons.image_outlined, color: Color.alphaBlend(theme.colorScheme.primaryContainer.withOpacity(0.15), theme.colorScheme.onPrimaryContainer)),
                    ),
        ),

CTalvio commented 9 months ago

How about this:

Media (image, and in the future, video posts): containerColor for background, onContainerColor for icon. Written media (links and text post): onContainerColor for background, containerColor for background

Do you mean primaryContainer and onPrimaryContainer? Those properties you refer to don't exist? Looks like you have a different colour completely vs the link icon though.

Yeah I wasn't using the exact names, just illustrating my idea, anyway, feel free to use that as a starting point. And yeah I did end up doing the link the other way around at first.

tom-james-watson commented 9 months ago

Yeah I will play around more because you still get very similar colours sometimes with your code:

CTalvio commented 9 months ago

Yup, some themes just straight up have the same color for the accents. Usually its the secondary and tertiary that are too similar, but sometimes it's the primary and secondary.

alphaBlend should give you some more options tho.

Edit: You'll probably get the most distinction most reliably, by using the primary and tertiary colors, and skipping the secondary, unlike what I did.

CTalvio commented 9 months ago

There's always the option of editing the actual themes. as well.

tom-james-watson commented 9 months ago

Been playing around and have two ideas.

One is to just do alpha blending like you suggest and have the three options be blends of primary with red, blue and green. This means you get colours that feel like that they fit in the app and are obviously different, without being too saturated or light or dark:

Another option that I also think might make more sense is to stop trying to find X different colour combinations for all the post types. What we're really trying to solve here is being able to more easily see which posts allow me to interact with the thumbnail. Therefore, it could just be a case of having one colour for interactive (images and later videos) posts and one colour for non-interactive (links and text):

Let me know your thoughts.

hjiangsu commented 9 months ago

Thanks for making this PR! These are my own thoughts on this:

I feel like using distinctive colours here seem to be the way to go. I don't think we can cover all of the Material You theming and be able to accommodate for all those use-cases. It'll be difficult to find a combination of primary/secondary/tertiary colours which don't look similar
Although using static colours mean that it no longer matches the Material You theme, this change would be a deliberate choice. Once users get used to the colours, it would help keep their expectations consistent no matter the colour theme (which is my personal preference for things like this)

If anything, we can potentially revisit this in the future (or you can feel free to try this out) to make some colour derivator which takes in a Material theme and outputs distinctive yet complementary colours. Some potential resources for this:

https://m3.material.io/theme-builder#/custom (see Extended Colours)
https://api.flutter.dev/flutter/package-material_color_utilities_blend_blend/Blend/harmonize.html (harmonize method which might be useful to generate a colour that matches another given colour)

tom-james-watson commented 9 months ago

Thanks for the input! Happy for me to update this PR with some static colours? Any strong opinions on what they are? If not, I will just play around with some sensible combinations (tomorrow).

CTalvio commented 9 months ago

Another option that I also think might make more sense is to stop trying to find X different colour combinations for all the post types. What we're really trying to solve here is being able to more easily see which posts allow me to interact with the thumbnail. Therefore, it could just be a case of having one colour for interactive (images and later videos) posts and one colour for non-interactive (links and text):

This is why I suggested making one category dark on bright, and the other bright on dark.

I really like the blended colors in those top two images, primary colors blended towards the current theme seem to work quite nicely, and the different shades of the same color remain recognizable, but also congruent with the app. This was my eventual plan as well, but I was unsure it could look good. It seems it could.

FYI, making these colors static is something I'd fork the project over, I've been wanting to make the static comment depth colors fit the theme, too.

By all means, make static UI colors an option, but personally this is a case where I will take aesthetics over functionality.

machinaeZER0 commented 9 months ago

Really liking the more differentiated colors overall! Definitely will help keep the different post types quickly identifiable at a glance.

This would be outside the scope of this PR, but I wonder if we would consider making the badges/icons sliiiightly smaller, if we end up making this change? Since the thumbnails in compact mode are themselves quite small, it would be great to see as much of the image as possible, and if we increase the diversity of the badge colors then users should quickly acclimate to which color means what (in addition to the icons that would still be present). Thoughts on that?

machinaeZER0 commented 9 months ago

FYI, making these colors static is something I'd fork the project over, I've been wanting to make the static comment depth colors fit the theme, too.

What will you be calling your fork?

CTalvio commented 9 months ago

FYI, making these colors static is something I'd fork the project over, I've been wanting to make the static comment depth colors fit the theme, too.

What will you be calling your fork?

Nothing. I'd just start maintaning my own version and build my own apks. Its not like I want to compete, just meet my own desires first and foremost.

micahmo commented 9 months ago

I wonder if it would work with user chips too. ☺

What do you mean by this by the way? I assume it would be a separate issue / PR but maybe I can take a look

@tom-james-watson Special users (OP, admin, mod) are identified by placing their username in a colored container (very similar to the post type indicators). I believe they suffer from the same problem where they're no longer very distinguishable. If you come up with a good mechanism here for the post indicators, I'm hoping you can apply the same change there.

https://github.com/thunder-app/thunder/blob/3a50c73f2eae98cb783d260c8ab3d99d48695c26/lib/post/widgets/comment_header.dart#L282-L285

Example:

Been playing around and have two ideas.

One is to just do alpha blending like you suggest and have the three options be blends of primary with red, blue and green. This means you get colours that feel like that they fit in the app and are obviously different, without being too saturated or light or dark:

My two cents, this is the best thing I've seen in the thread. The colors are completely distinct while following the theme. Obviously you just posted two examples, but if the same holds true for other theme, I'd say go for it!

I wonder if we would consider making the badges/icons sliiiightly smaller

+1 to this as well! Obviously we're not asking @tom-james-watson to do extra work when he has already gracious dived into improving the colors. But yes, this is something I'd like to see too.

machinaeZER0 commented 9 months ago

FYI, making these colors static is something I'd fork the project over, I've been wanting to make the static comment depth colors fit the theme, too.

What will you be calling your fork?

Nothing. I'd just start maintaning my own version and build my own apks. Its not like I want to compete, just meet my own desires first and foremost.

Kinda sounds like you'd just like a toggle (similar to the Material You toggle) that themes everything more broadly than Accent Color currently does? Feel like that would satisfy all parties, if implemented thoughtfully :)

machinaeZER0 commented 9 months ago

Obviously we're not asking @tom-james-watson to do extra work when he has already gracious dived into improving the colors.

100%! Thank you for taking interest in Thunder, Tom :D

tom-james-watson commented 9 months ago

I've given this some more thought and I agree with what looks like the general consensus - that the version with the alpha blends of red/blue/green is the nicest solution to this for now. It means that you both have consistent colouring of post types (if not 100% static), but you still keep the colours harmonious with the rest of the theme. I also switched which colours are used where to make links and images have the most contrast (red vs blue), because most of the time "Show Text Post Indicator" will be off.

I've pushed a commit that introduces this change. I'm happy with how it looks when switching between dark/light and all of the different themes. There are a few screenshots below for reference.

Let me know if you're happy with this!

100%! Thank you for taking interest in Thunder, Tom :D

Thank you to you guys for making it! It's been a great experience so far ❤️

+1 to this as well! Obviously we're not asking @tom-james-watson to do extra work when he has already gracious dived into improving the colors. But yes, this is something I'd like to see too.

I'm happy to keep working on a few things like this. I can try and take a look as a follow-up.

CTalvio commented 9 months ago

The color blend looks so good! We should totally use it for user chips, and perhaps even comment indent colors, eventually.

tom-james-watson commented 9 months ago

I can maybe look at that after this PR. We could split out these new functions into something more generic.

machinaeZER0 commented 9 months ago

Beautiful!

hjiangsu commented 9 months ago

FYI, making these colors static is something I'd fork the project over, I've been wanting to make the static comment depth colors fit the theme, too.

@CTalvio Maybe I can clarify what I meant - I think the use of "static" was incorrect from my previous response. What I meant was pretty much what @tom-james-watson did with the alpha blends but with red/green/blue! The static part was referring to the red/green/blue portion of it. If I'm not mistaken, blending these red/green/blue colours with the theme colours would push that given colour towards that respective hue (which might not end up completely matching the Material You theme).

However, it does make it distinct in the sense that image indicators will always be pushed towards a more red hue, or post indicators will always be pushed towards a greener hue. So no matter what theme or accent you choose, you can expect the different indicators to be pushed more towards their respective hues.

I've given this some more thought and I agree with what looks like the general consensus - that the version with the alpha blends of red/blue/green is the nicest solution to this for now. It means that you both have consistent colouring of post types (if not 100% static), but you still keep the colours harmonious with the rest of the theme

I think this looks really good too! Thanks for trying out different combinations and inputting our feedback :D I think this is the way to go as well. Let me know if you're done playing with this and I can go ahead and approve/merge this in

tom-james-watson commented 9 months ago

Yeah, I'm done and happy with the current state - sounds like you guys are too 🎉

CTalvio commented 9 months ago

I think we should merge this now, then, so users can get used to the red green blue and not have it change on them in the release after this one.

@hjiangsu?

Also yes, I'd been thinking of the color blend option for a while, but had not explored it yet. I only mentioned it in matrix.

Clearly it works great!

hjiangsu commented 9 months ago

Already ahead of you! :D

thunder-app / thunder