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

CRM-21098 #10892

Merged
merged 2 commits into from
Aug 25, 2017
Merged

CRM-21098 #10892

merged 2 commits into from
Aug 25, 2017

Conversation

herbdool
Copy link
Contributor

@herbdool herbdool commented Aug 23, 2017

Overview

Prevent Backdrop admin menu dropdowns appearing beneath the CiviCRM admin menu by hiding the Backdrop admin menu when the CiviCRM admin menu is visible and also to show the Backdrop admin menu when the CiviCRM admin is hidden.

Before

Backdrop admin menu is visible beneath the CiviCRM admin menu.

backdrop-civicrm-menus

After

Backdrop admin menu is hidden when CiviCRM admin menu is visible.

Technical Details

Adds a crm.backdrop.js file for Backdrop installs.

Comments

This PR might need to be slightly updated if #10891 is merged first.


@@ -0,0 +1,14 @@
// http://civicrm.org/licensing
CRM.$(function($) {
$(document).on('crmLoad', function(e) {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need this wrapper. We're already waiting for document.ready because of the CRM.$(function()... construct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// http://civicrm.org/licensing
CRM.$(function($) {
$(document).on('crmLoad', function(e) {
$('#admin-bar').css('display', 'none');
Copy link
Member

Choose a reason for hiding this comment

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

Simpler to use the .hide() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason I don't get the same results when using .show() and .hide(). I can't get it to hide the Backdrop admin bar on page load with .hide() and using .show() on the clicks seems to interfere with the page load as well.

$('#admin-bar').css('display', 'none');
});
$('.crm-hidemenu').click(function(e) {
$('#admin-bar').css('display', 'block');
Copy link
Member

Choose a reason for hiding this comment

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

Use the .show() method.

$('.crm-hidemenu').click(function(e) {
$('#admin-bar').css('display', 'block');
});
$('#crm-notification-container').click(function(e) {
Copy link
Member

Choose a reason for hiding this comment

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

Better to do $('#crm-notification-container').on('click', '#crm-restore-menu', function() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I was wondering how to attach it to #crm-restore-menu.

*/
public function appendCoreResources(&$list) {
$list[] = 'js/crm.backdrop.js';
}
Copy link
Member

Choose a reason for hiding this comment

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

Yes this function is redundant now.

@herbdool
Copy link
Contributor Author

@colemanw ready for another review. I've had mixed results with using .hide()/.show() compared to using .css() for this. I'm not sure why.

@colemanw
Copy link
Member

@herbdool I just pushed a change to the restore menu click handler. Give it a try on your local to ensure it works.

@herbdool
Copy link
Contributor Author

@colemanw oops, thanks for restoring that. It works for me locally.

@colemanw colemanw merged commit cedaef1 into civicrm:master Aug 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants