pbaity / rocketchat-dark-mode

An easy user-togglable dark mode for Rocket.Chat
MIT License
367 stars 125 forks source link

3.13.x - Breadscrumbs, code in attachements, poll.bot #134

Closed fdellwing closed 3 years ago

fdellwing commented 3 years ago

I noticed no broken generated classes for the 3.13.x release.

fdellwing commented 3 years ago

@TBG-FR and @pbaity

Please take a good and open look at this.

pbaity commented 3 years ago

@fdellwing Thanks for the awesome work with this PR and all the issues you've been responding to! I've been very busy elsewhere lately but I should be able to look at this today. If it closes any issues, list them as closes.

fdellwing commented 3 years ago

I just added two new rules:

  1. Unset the background color in breadcrumbs to use the header background color
  2. Fix the font color for the poll app
TBG-FR commented 3 years ago

Thanks for the hard work ! I'll get my eyes on that today, and try it ;)

TBG-FR commented 3 years ago

Looks good to me, I agree with you that it's the correct way to go, it makes more sense.

The "active" color in dropdowns still looks a bit bright to me, but apart from that, I don't have anything against that improvement !

pbaity commented 3 years ago

@fdellwing @TBG-FR Finally getting around to testing this out myself. Your fixes look good, but I'm not yet convinced that changing the background color of the messages is good - I like the higher contrast behind the messages, and every other messaging app I've used with a dark theme uses a different background color for any panels or menus and the main messages area (cf. Slack, Microsoft Teams, Discord, etc). Using the same color makes the styling look flat and, frankly, broken.

That said, since both of you were positive about this change, I'm interested in hearing your reasons. If it's just a matter of personal preference, that's fine, but I want to give you the chance to make your case. If needed, we can separate the background color changes into a separate PR so we can merge the other fixes here while we discuss the background colors.

fdellwing commented 3 years ago

My main problem is, that the two colors are to different. Also the contrast is to high for me, but that is personal preference.

I could without a problem live with two different colors, if they are similar. A good example for this is imho Discord.

Screenshot 2021-04-16 210510

It uses different shades of grey, but they are all similar enough to look like a single application.

Additionally, some changes I did here (like the buttons in the room header) only work with a lighter grey background and not with the near black that is currently used.

pbaity commented 3 years ago

That I can understand, and you're right that Discord is a good example - my only concern is making the app look flat my making too many things the same color, or colors so close in shade that they don't give any depth to guide the eyes. But we can work on that.

What do you think of the colors previewed in Rocket.Chat's native dark theme pictured here? If you like these better, we can capture those colors and use them, or even bring them a little closer together in shade. I'm not attached to the color hex codes in the repo now, so feel free to change those codes too.

iamjameswalters commented 3 years ago

I think following upstream's colors would be a wise choice. I happen to like them and think they strike a good balance for what we're discussing here in terms of contrast etc., but it also lays the groundwork for a more seamless tradition to the native solution once it lands.

</twocents>

TBG-FR commented 3 years ago

@pbaity I agree with you on the constrat, because I like it personnaly, but I've had some people telling me it was too "high"/"crude" for them...

Discord is definitely a good example, as is the idea of using RocketChat native dark theme colors, especially because it will make a smoother transition when we'll switch to it, as @iamjameswalters said !

pbaity commented 3 years ago

@fdellwing I know it's extra trouble, but I think this is a good discussion and I think it'd be helpful to move the color changes to another PR, which will focus exclusively on the color change. That way I can merge these fixes, and we can have a single PR to point to that updates and improves the colors.

If you want me to do that work instead, I can, but I want you to get the credit for the contribution if possible.

fdellwing commented 3 years ago

The native theme looks not perfect but a lot better than the current state (sorry).

I'll split the changes that do not work with the current color into a seperate branch, but it might take till I'm back at work on monday.

pbaity commented 3 years ago

The native theme looks not perfect but a lot better than the current state (sorry).

No offense taken 😄 I mostly kept the colors used by the gist I forked almost a year and a half ago, and I'm no designer - I'm relying on the community to bring these improvements, which is exactly what you're doing. So thank you!

fdellwing commented 3 years ago

I left only the code that works with the old colors, will do a new branch for reworking of colors.

pbaity commented 3 years ago

Does this still fix #131? Looks like no when I test it out, just wondering. If there are any other issues it fixes let me know, otherwise looks good.

fdellwing commented 3 years ago

No it does not. It only fixes things I notices myself:

pbaity commented 3 years ago

Thanks @fdellwing , great job!