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

Configurable menubar position #21

Merged
merged 8 commits into from Nov 26, 2018
Merged

Configurable menubar position #21

merged 8 commits into from Nov 26, 2018

Conversation

colemanw
Copy link
Collaborator

@colemanw colemanw commented Nov 25, 2018

Overview

Adds a system setting to control menubar placement; works across the different CMSs.

screenshot from 2018-11-25 12-07-40

Background

There has been some discussion over the years about where the CiviCRM menubar should go, and different CMSs have gotten different treatment. Currently WP, Drupal & Backdrop get the menubar fixed to the top of the screen, obscuring the CMS admin menu. Some users think that's a good use of space, others find it annoying. Currently on Joomla the menubar is at the top of the content area and scrolls with the page; again with pros and cons.

In overhauling the menubar system and optimizing it for each CMS, I couldn't find one perfect solution that worked for every CMS, and even within a single CMS there may be different themes and admin interfaces which make menubar placement more complicated.

Technical details

Instead of one hardcoded position per CMS, we now have three options with the possibility of more.
As noted in the updated readme, the selected option becomes a css class added to the page body. The CRM.menubar.initialize() function places the menubar markup inside #crm-container instead of body if position is set to "Above Content Area". The stylesheet does the rest.
Since it wasn't possible to style this in a 100% generic way, an additional stylesheet is included for fine-tuning each CMS.

Notes

This PR also improves styling of the mobile menu to make it look good on various CMSs. E.g. on Wordpress:

screenshot from 2018-11-25 12-27-51

@mattwire
Copy link
Contributor

@vingle Configurable civicrm menu positions!

@colemanw colemanw force-pushed the setting branch 2 times, most recently from 416fd39 to 11b902a Compare November 25, 2018 23:24
@vingle
Copy link
Contributor

vingle commented Nov 25, 2018

@vingle Configurable civicrm menu positions!

Great solution — and works perfectly on Joomla.

@christianwach
Copy link

@colemanw What do I have to do to get the dropdown to show up?

@vingle
Copy link
Contributor

vingle commented Nov 26, 2018

@colemanw What do I have to do to get the dropdown to show up?

did you go Administer -> Customize Data & Screens -> Display Preferences?

@christianwach
Copy link

Yep, option doesn't seem to be rendered:

screen shot 2018-11-26 at 11 48 40

@christianwach
Copy link

Nothing in the logs to suggest there's a problem.

@mattwire
Copy link
Contributor

@christianwach Did you clear civicrm caches? ie. civicrm/clearcache It's a new setting so won't show until that's done

@christianwach
Copy link

@mattwire Of course!

@mattwire
Copy link
Contributor

@christianwach Check if Civi thinks the setting exists ('menubar_position') using the settings API. And looks like it's being added using a .extra.tpl so can you check if that is being included?

@colemanw
Copy link
Collaborator Author

colemanw commented Nov 26, 2018

@christianwach if you have any custom code on your site which already implements a /CRM/Admin/Form/Preferences/Display.extra.tpl then that could be the problem. That file is just a temporary hack until we get this migrated to core, but those .extra.tpl files are known to conflict with each other if there is more than one with the same name.

You can also set it with the api explorer:

screenshot from 2018-11-26 09-11-11

@colemanw
Copy link
Collaborator Author

@vingle thanks for testing on Joomla! Can you fill out that section of #16 ?

@vingle
Copy link
Contributor

vingle commented Nov 26, 2018

@vingle thanks for testing on Joomla! Can you fill out that section of #16 ?

Will do asap. Incidentally one issue I noticed that I've not had time to test further or write an issue up for was the positioning of the divider lines seemed wrong comparing with the original menu.. (ie the line is below 'new a/b test' / 'new sms' rather than above it)

image

@colemanw
Copy link
Collaborator Author

@vingle whoops you're right, my mistake. Fixed: b6830fc

@colemanw
Copy link
Collaborator Author

I'll merge this so there's no confusion about which branch to test when people engage with #16

@colemanw colemanw merged commit 4baa4f9 into master Nov 26, 2018
@christianwach
Copy link

Okay, so finally I got this working by upgrading to CiviCRM 5.9.alpha1 and uninstalling/reinstalling KAM. Simply couldn't get it to work with a simple upgrade.

I like the general idea of giving people the choice of where the menu appears. I'll have a go at refining the implementation when I get some free time. For example:

  • The menu and element heights ought to be 32px for desktop viewports and 46px for viewports below 768px.
  • The WordPress admin menu has position: fixed desktop viewports and position: absolute below 600px. This should be respected or else we get this as we scroll: screen shot 2018-11-27 at 10 59 58
  • I'm not sure that adding a margin-top to #wpbody is the best way to make space for the CiviCRM menu - it might be better to do what WordPress does and add extra padding-top to the <html> element and avoid the blank space above "Dashboard" in the left-hand menu: screen shot 2018-11-27 at 11 05 04

FWIW there are some other oddities, but I suspect they're inherent to Smart Menus itself. Here's what happens when I have a viewport that's not tall enough to hold the submenus:

screen shot 2018-11-27 at 10 51 02

As you can see, the items below "System Settings" disappear.

Having said all of the above, this is really nice work @colemanw and an excellent basis for future development! 👍

@colemanw colemanw deleted the setting branch November 29, 2018 03:44
@colemanw
Copy link
Collaborator Author

@christianwach I think I've addressed your major concerns in #22.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants