-
Notifications
You must be signed in to change notification settings - Fork 125
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
3.13.x - Breadscrumbs, code in attachements, poll.bot #134
Conversation
Update fork
@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. |
I just added two new rules:
|
Thanks for the hard work ! I'll get my eyes on that today, and try it ;) |
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 ! |
@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. |
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. 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. |
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. |
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.
|
@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 ! |
@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. |
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. |
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! |
I left only the code that works with the old colors, will do a new branch for reworking of colors. |
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. |
No it does not. It only fixes things I notices myself:
|
Thanks @fdellwing , great job! |
I noticed no broken generated classes for the 3.13.x release.