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

Better support for hookable menubar colors #14944

Merged
merged 2 commits into from Aug 2, 2019

Conversation

@colemanw
Copy link
Member

commented Aug 1, 2019

Overview

This makes it easier for themes such as Shoreditch to adjust menubar colors conditionally so as not to override user settings.

Before

Only base menu color could be changed via hook.

After

Base menu color, highlight color, text color, etc can all be changed via hook.

Technical Details

This also includes some general improvements to CRM_Utils_Color, making it easier to work with color settings. E.g. you can now set the menubar color to "green" and it will automatically be converted to the correct hex value. Tests included.

@civibot

This comment has been minimized.

Copy link

commented Aug 1, 2019

(Standard links)

@civibot civibot bot added the 5.16 label Aug 1, 2019

@colemanw colemanw added the has-test label Aug 1, 2019

@colemanw

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@eileenmcnaughton I targeted this one for the RC based on your comments on #14924.

@colemanw colemanw referenced this pull request Aug 1, 2019
}
$params = $e->params;
// "color" is deprecated in favor of the more specific "menubarColor"
$menubarColor = $params['color'] ?? $params['menubarColor'];

This comment has been minimized.

Copy link
@eileenmcnaughton

eileenmcnaughton Aug 1, 2019

Contributor

If color (sic) is deprecated why is it the 'first choice' here?

This comment has been minimized.

Copy link
@colemanw

colemanw Aug 1, 2019

Author Member

Because if someone has implemented this hook to alter the "color" param, then we should continue to honor that. Otherwise we use the new param.

This comment has been minimized.

Copy link
@eileenmcnaughton
@kcristiano

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@eileenmcnaughton

This looks good to merge.

Did an r-run on both Drupal and WP. With and Without Shoreditch. Shoreditch looks to be able to override any menu color changes.

Without Shoreditch it works as expected.

The only concern is if a user attempts to change the menu color when an extension is overriding it they may be driven mad when it does not work as there is no indication that changing is not possible.

Can we add a documentation task on this?

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@colemanw this is good to merge from my POV once you have considered @kcristiano feedback

@colemanw

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

The only concern is if a user attempts to change the menu color when an extension is overriding it they may be driven mad when it does not work as there is no indication that changing is not possible.
Can we add a documentation task on this?

So this PR is aimed at avoiding that kind of user frustration so that themes can check settings before overriding colors rather than just clobbering them with css rules, but the theme has to implement it in a conditional way. I agree to writing some documentation about the correct way to implement the hook so that user preferences are respected.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

OK merging - leaving you to follow up with docs

@eileenmcnaughton eileenmcnaughton merged commit 38d6407 into civicrm:5.16 Aug 2, 2019

1 check passed

default Build finished.
Details

@eileenmcnaughton eileenmcnaughton deleted the colemanw:color branch Aug 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.