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

Decoupled flyout region #59

Open
wants to merge 44 commits into
base: develop
Choose a base branch
from

Conversation

darko-hrgovic
Copy link

Decoupled flyout region on top of drawer nav in develop branch

…primary nav is a good idea if choosing not to use the primary nav in the drawer, otherwise primary nav goes away entirely
…s by creating a separate containing div for the flyout
… where secondary menu was not expanding on click
Copy link
Collaborator

@joelpittet joelpittet left a comment

Choose a reason for hiding this comment

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

Any chance you could toss in a photo of what this is in the pull request?

@@ -4,3 +4,5 @@ node_modules/

# Postcss-fied CSS files
postcss/

.DS_Store
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest moving some of the clean-up changes to their own issue so they can be reviewed as one thing.

Copy link
Author

Choose a reason for hiding this comment

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

Understood.

Copy link
Author

Choose a reason for hiding this comment

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

I've pushed the changes wrt to your comments, Joel. Thanks very much for reviewing.

@@ -224,7 +233,7 @@

</div><!-- /#content -->
</div><!-- /#main -->
<?php print $fluidcontainerend; ?>
<?php // print $fluidcontainerend; ?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

These comments worry me a bit, usually don't like leaving commented out code.

Copy link
Author

Choose a reason for hiding this comment

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

We override this in our child theme because it breaks pages that want full width backgrounds, but these should be uncommented in megatron, which I've done in my flyout branch and will do in develop fork as a new pull request.

@@ -100,26 +100,6 @@ function megatron_form_system_theme_settings_alter(&$form, &$form_state) {
),
);

$form['clf_navigation_option']['clf_sticky_option'] = array(
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened to this hunk of code? I don't see "clf_navoption" for example in the new code hunks

Copy link
Author

Choose a reason for hiding this comment

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

This was moved down with the other $form['clf_navigation_option'] options

@@ -184,7 +164,7 @@ function megatron_form_system_theme_settings_alter(&$form, &$form_state) {
$form['clf_navigation_option']['clf_use_primary_menu_in_drawer'] = array(
'#type' => 'checkbox',
'#title' => t('Use the primary menu in the off-canvas drawer?'),
'#description' => t('This is optional in case you want to use additonal content blocks, such as a menu block, in the off-canvas drawer region.'),
'#description' => t('If not chosen, you should use a menu block or alternate method for main navigation in the off-canvas drawer region.'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that the wording could use clarification, I have a hang-up on "chosen" but otherwise +1

Copy link
Author

Choose a reason for hiding this comment

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

I changed it to "If you do not use the primary menu in the drawer, you should use a menu block or alternate method for main navigation in the off-canvas drawer region." Hope this is better.

@occupant occupant mentioned this pull request Jun 18, 2019
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.

3 participants