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

Colorize checkboxes depending on theming color #415

Merged
merged 7 commits into from
Jul 28, 2016

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jul 15, 2016

partly fixes #378

  • checkboxes match theme color
  • checkmark-white.svg replaced to match checkmark.svg

2016-07-15-144933_535x208_scrot

@mention-bot
Copy link

@juliushaertl, thanks for your PR! By analyzing the annotation information on this pull request, we identified @jancborchardt, @Kondou-ger and @jbtbnl to be potential reviewers

@juliusknorr juliusknorr changed the title Colorize checkboxes depending on theming color [WIP] Theming: Colorize checkboxes depending on theming color Jul 15, 2016
@nickvergessen
Copy link
Member

#412 is merged, want to continue? 😀

@juliusknorr
Copy link
Member Author

@nickvergessen Sure, I will have some time tomorrow to go on with this.

@jancborchardt
Copy link
Member

We need to take into account what happens when someone uses white as the color, or very light colors in general which are not very easy to see on the white background. In that case I’d say we either use the default blue or a standard grey.

@nickvergessen nickvergessen mentioned this pull request Jul 18, 2016
2 tasks
@juliusknorr juliusknorr force-pushed the theming-colorize-checkboxes branch from 4977522 to eef8882 Compare July 19, 2016 12:41
@juliusknorr
Copy link
Member Author

Should be ready for review now.

  • Checkbox icon will always stay white, because this looks way better than black, even on bright colors
  • Color of the checkboxes will default to grey, if color gets to bright

@nextcloud/designers please check if the boundary for changing to grey is good or if it needs to be changed

I also changed the way the css is built, I hope the string concatenation is not that bad, as the response is cached anyway.

@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 19, 2016
@juliusknorr juliusknorr changed the title [WIP] Theming: Colorize checkboxes depending on theming color Theming: Colorize checkboxes depending on theming color Jul 19, 2016
@nickvergessen
Copy link
Member

nickvergessen commented Jul 19, 2016

Some comments:

  • You can hardcode PHP_EOL to \n
  • If your theme is gray (e.g. cdcdcd) disabled and active boxes look very similar, should we special case this?
  • Radio buttons seem to not be covered? Maybe Checkboxes owncloud/core#23063 (comment) helps to cover everything
  • Disabled+checked checkboxes have a colored border which looks a bit odd.

@nickvergessen nickvergessen changed the title Theming: Colorize checkboxes depending on theming color Colorize checkboxes depending on theming color Jul 19, 2016
@juliusknorr
Copy link
Member Author

Good point, I will add the other states and radio buttons tomorrow.

At the moment, I don't have a good solution for separating the disabled icon from the checked one, so that they don't look similar with a gray theme color, but I will think about that. Maybe @jancborchardt ?

@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 19, 2016
@MorrisJobke
Copy link
Member

At the moment, I don't have a good solution for separating the disabled icon from the checked one, so that they don't look similar with a gray theme color, but I will think about that. Maybe @jancborchardt ?

Then leave it for now as it is. We could iterate on this later.

@MorrisJobke MorrisJobke added this to the Nextcloud 11.0 milestone Jul 22, 2016
@juliusknorr
Copy link
Member Author

Ok, I will have time after the weekend to continue with this pr.

@juliusknorr juliusknorr force-pushed the theming-colorize-checkboxes branch 3 times, most recently from 1d3f4b6 to 0b2a679 Compare July 25, 2016 15:51
@juliusknorr
Copy link
Member Author

Radio buttons are now also colorized. Please review again.

Normal look:
boxes_1

Look on bright colors:
boxes_2

I'm not sure if generating the SVG for the radio buttons is the best way, at the moment this is more a hacky approach, but my first plan to use mask-image CSS failed, as this is also unsupported in Firefox. @jancborchardt sorry for giving wrong information on that.

@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 25, 2016
@juliusknorr
Copy link
Member Author

Yes, we can change the svg-thing later, if someone has a idea to solve this in a more elegant way.

@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 27, 2016
@MorrisJobke
Copy link
Member

MorrisJobke commented Jul 28, 2016

Nice! Tested and works (IE11, Edge, Firefox, Safari, Chrome) 👍

@jancborchardt
Copy link
Member

Awesome, works really well! 👍 :)

@jancborchardt jancborchardt merged commit 9ebd091 into master Jul 28, 2016
@jancborchardt jancborchardt deleted the theming-colorize-checkboxes branch July 28, 2016 09:12
@schiessle
Copy link
Member

schiessle commented Jul 28, 2016

@juliushaertl do you want to prepare a backport to sable10? Would be nice to have it in Nextcloud 10 and we also need it for #620 to ship the new first run wizard.

@juliusknorr
Copy link
Member Author

@schiessle Yes, but i'll wait until #622 is merged.

@MorrisJobke
Copy link
Member

@schiessle Yes, but i'll wait until #622 is merged.

Was merged. @juliushaertl Could you create the backport?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. enhancement feature: theming
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Theming: coloring of folders and log in background
7 participants