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.

Sign up for GitHub

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

Jump to bottom

Theming: Fix missing color usage #771

Merged
merged 5 commits into from
Aug 26, 2016
Merged

Theming: Fix missing color usage #771

merged 5 commits into from
Aug 26, 2016

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Aug 8, 2016 •
edited
Loading

  • Theming: colorize primary buttons
  • Remove internal from getThemingDefaults, maybe @LukasReschke can check if that makes sense
  • Theming: colorize jquery ui elements jQuery UI not affected by Theming #830
  • Theming: fix federated sharing button color
  • federated sharing button text color / custom logo

requires #1026

@juliushaertl juliushaertl added this to the Nextcloud 11.0 milestone Aug 8, 2016
@juliushaertl juliushaertl mentioned this pull request Aug 10, 2016
Closed
@jancborchardt jancborchardt mentioned this pull request Aug 11, 2016
Closed
10 tasks
This was referenced Aug 19, 2016
Closed
Merged
@jancborchardt
Copy link
Member

cc @Mar1u5 @nickvergessen @nextcloud/designers @nextcloud/theming please help reviewing :)

@juliushaertl
Copy link
Member Author

@jancborchardt still not complete for review ;) - we need #1026 for all the remaining theming pull requests. But i'll rebase this one later today and make it ready.

@juliushaertl juliushaertl added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 25, 2016
@juliushaertl juliushaertl force-pushed the theming-fixes branch 2 times, most recently from 6870e43 to 6b68f3b Compare August 25, 2016 11:19
@juliushaertl
Copy link
Member Author

CI is happy, ready for review @nextcloud/designers @nextcloud/theming

I've included #1026 for easier testing, will rebase to master once this is merged.

nickvergessen
nickvergessen reviewed Aug 25, 2016
View reviewed changes
apps/federatedfilesharing/settings-personal.php
$theme = \OC::$server->query("ThemingDefaults");
$color = $theme->getMailHeaderColor();
$textColor = "#ffffff";
if($theme instanceof \OCA\Theming\ThemingDefaults) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw an Exception if the theming app is disabled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickvergessen Hmm, I've disabled it, but there was no Exception for me.

Sould I just check if the app is enabled like this:

if(\OC::$server->getAppManager()->isEnabledForUser("theming")) { ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's better since it's more explicit.

@juliushaertl
Copy link
Member Author

@nickvergessen Thanks for the comments. I've just pushed an update.

nickvergessen
nickvergessen reviewed Aug 25, 2016
View reviewed changes
apps/federatedfilesharing/settings-personal.php
$textColor = "#ffffff";
if(\OC::$server->getAppManager()->isEnabledForUser("theming")) {
$logoPath = $theme->getLogo();
$util = \OC::$server->query("\OCA\Theming\Util");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could throw an exception OCP\AppFramework\QueryException, better catch it so the page still displayes 😉

@juliushaertl
Copy link
Member Author

@nickvergessen ok, i've added a try-catch-block as well.

@oparoz
Copy link
Member

oparoz commented Aug 26, 2016

Does that fix media type icons? That's the biggest problem right now.

@LukasReschke
Copy link
Member

@oparoz #840

@oparoz
Copy link
Member

oparoz commented Aug 26, 2016

Awesome, thanks.

@rullzer
Copy link
Member

rullzer commented Aug 26, 2016

#1026 is in. @juliushaertl can you rebase?

@juliushaertl
Copy link
Member Author

@rullzer done.

@nickvergessen
Copy link
Member

👍 works and looks nice. Thanks!

nickvergessen
nickvergessen reviewed Aug 26, 2016
View reviewed changes
apps/federatedfilesharing/templates/settings-personal.php
@@ -66,7 +65,7 @@ class='js-gs-share social-gnu'>
<?php p($l->t('HTML Code:')); ?>
<xmp><a target="_blank" rel="noreferrer" href="<?php p($_['reference']); ?>"
style="padding:10px;background-color:#0082c9;color:#fff;border-radius:3px;padding-left:4px;">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to use the colors here? It still uses blue+white instead of what the preview shows.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah also forgot to apply the style changes here. I'll fix that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickvergessen ok, i've updated the html code as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nickvergessen
Copy link
Member

Second review, @rullzer ?

@rullzer
Copy link
Member

rullzer commented Aug 26, 2016

LGTM

@rullzer rullzer merged commit 53725d4 into master Aug 26, 2016
@rullzer rullzer deleted the theming-fixes branch August 26, 2016 13:02
@juliushaertl juliushaertl mentioned this pull request Sep 14, 2016
Closed
@MorrisJobke MorrisJobke mentioned this pull request Sep 21, 2016
Closed
67 tasks
@MorrisJobke MorrisJobke mentioned this pull request Aug 18, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement feature: theming
Projects
None yet
Milestone
Nextcloud 11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@juliushaertl @jancborchardt @oparoz @LukasReschke @rullzer @nickvergessen