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

Menubar - Improve flexibility & remove hardcoded values #14839

Merged
merged 2 commits into from
Jul 19, 2019

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 18, 2019

Overview

Minor tweak to crm.menubar.js to make it less reliant on the presence of a <div id="crm-container"> which may not be present on non-civicrm pages.
Also tweaks the js to remove hardcoded pixel values to work better with theme overrides.

Before

Problems loading on non-civicrm pages.
Hardcoded values interfere with theme overrides.

After

Works better on non-civicrm pages.
Works better with theme overrides to the menu size & breakpoints.

Comments

Fixes https://www.drupal.org/project/civimenu/issues/3068607
Fixes https://lab.civicrm.org/dev/core/issues/1127

@civibot
Copy link

civibot bot commented Jul 18, 2019

(Standard links)

@civibot civibot bot added the master label Jul 18, 2019
@colemanw colemanw changed the title Menubar - Use class instead of id for flexibility Menubar - Improve flexibility & remove hardcoded values Jul 18, 2019
@totten
Copy link
Member

totten commented Jul 18, 2019

r-code : Looks much nicer without the magic numbers. 👍

@AkA84, any chance you'll be able to look at this?

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Jul 19, 2019
@colemanw
Copy link
Member Author

AkA84 tested this & approved it on the gitlab issue. Looks to be merge ready.

@mattwire
Copy link
Contributor

It would be good to see an r-run on Joomla / Wordpress + with mosaico editor before merging to ensure this doesn't cause any regressions there. @vingle @lcdservices @kcristiano Are you able to do any of those checks?

@kcristiano
Copy link
Member

@colemanw I am unsure about the 'non-civicrm' is that a Drupal Only thing? In WP the menu looks fine after applying the patch.

I do think the only way to really test this in the wild is to merge it and monitor the 5.17 RC.

@mattwire
Copy link
Contributor

Merging based on @kcristiano comments

@mattwire mattwire merged commit a6c9f40 into civicrm:master Jul 19, 2019
@colemanw colemanw deleted the Menubar branch July 19, 2019 15:36
@colemanw
Copy link
Member Author

@kcristiano yes there is a drupal module to display the Civi menubar on selected non-civi pages. It wouldn't be hard to create a WP plugin to do the same.

davialexandre pushed a commit to compucorp/civicrm-core that referenced this pull request Jul 26, 2019
@AkA84
Copy link

AkA84 commented Jul 30, 2019

@colemanw what version is this PR going to be in? 5.16 or later?

@mattwire
Copy link
Contributor

@AkA84 It's in master (so 5.17) but I think it would make sense to backport for 5.16 if @colemanw agrees - as then we have all "theme related" changes in 5.16 including the new theming subsystem. And you can target 5.16 with shoreditch.

@colemanw
Copy link
Member Author

Ok I've opened up a backport PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants