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

Dark theme fixes #1451

Merged
merged 7 commits into from
Jan 17, 2019
Merged

Dark theme fixes #1451

merged 7 commits into from
Jan 17, 2019

Conversation

jancborchardt
Copy link
Member

Before:
dark theme chat sidebar before

After:
dark theme chat sidebar files

Please review @danxuliu @nextcloud/designers :)

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt jancborchardt added 3. to review bug design feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents labels Jan 17, 2019
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
… notifications

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Copy link
Member Author

jancborchardt commented Jan 17, 2019

Fixed some additional things, and there’s also further issues which someone else needs to check out though:

  • All icons in the sidebar which are on a background should be white, not black
    icons in sidebar should be white
  • Same for the create conversation menu
    top icons in menu should be white
  • And for the share menu (it is correct in the share dialog itself, and for the other icons)
    participant icons should be white
  • Password protect icon should be white
    password icon should be white

@jancborchardt jancborchardt changed the title Fix Chat sidebar in Files for Dark theme Dark theme fixes Jan 17, 2019
@nickvergessen nickvergessen self-assigned this Jan 17, 2019
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member

I'm not sure which share menu you are talking about?
If you talk about the "add participant" menu, that was fixed too by me, but I didnt tick it, since im not sure.

On the password thingy I don't know what's wrong. we use the correct class:

@include icon-black-white('no-password', 'spreed', 1);

' <span class="button {{#if hasPassword}}icon-password"{{else}}icon-no-password{{/if}}"></span>' +

Also once a password is set, it works as expected too.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member

Okay, found the problem. The regex in https://github.com/nextcloud/server/blob/20519434879dd0ee9278d0871d92e9c11295adfd/lib/private/Template/IconsCacher.php#L188 doesn't match if there is a transformation as well:
https://regex101.com/r/booyc4/1

I removed the transformation for now and adjusted the viewport, I guess it still looks the same, at least I couldn't see a difference. but maybe you @jancborchardt or @skjnldsv know more?

@skjnldsv
Copy link
Member

@nickvergessen which icon are we talking about?
Also: yes we then need to adapt the regex :)
Could you help?

@nickvergessen
Copy link
Member

The image is in 2ee1b41

I'm very bad at regexes, especially with look-ahead and stuff, but I can try

… dark theme

Actual fix is adjusting the regex on the server, allowing commas in the first regex:
nextcloud/server#13650

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member

Right, fixed in the server with nextcloud/server#13650

But also fixed in the app by adding fill="#000" because the actual color replacement works then. so will be fixed with either next release

@jancborchardt
Copy link
Member Author

jancborchardt commented Jan 17, 2019

Looks all good with your fixes @nickvergessen, thanks a lot! :) 👍 (Can’t Approve my own pull request ;)

@nickvergessen
Copy link
Member

/backport to stable15

@backportbot-nextcloud
Copy link

backport to stable15 in #1452

@danxuliu
Copy link
Member

Since 95e69a3 non-avatar icons are shown in black instead of white with the normal theme. Is that intended?

Conversation list:
talk-icon-black-conversation-list

Create conversation:
talk-icon-black-create-conversation

Add participant:
talk-icon-black-add-participant

@nickvergessen
Copy link
Member

Yes had to, the other apps do this since a long time already, i tried to block it because i think white looks more beautiful, but on dark mode nothing is visible otherwise

@jancborchardt
Copy link
Member Author

Hmm, @nickvergessen I agree with you that white looks more beautiful (and is in line with our placeholder images with the initial).

On dark mode it should also be white – we probably need to add an exception in the dark theme so that icons in these circles are never inverted. (We have the same issue for icons in primary buttons, like for creating a new user in the user management. Icons in primary buttons should also be white in both normal and dark theme.)

@skjnldsv @juliushaertl what do you think? A rule in the color inversion to skip the icons inside other forms (at least if it’s standards like these avatar-like elements, or primary buttons) would probably be good?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug design feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents help wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants