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

Add Navigation styling to BCB #3460

Merged
merged 10 commits into from
Mar 16, 2021
Merged

Conversation

pbking
Copy link
Contributor

@pbking pbking commented Mar 12, 2021

This change just adds configuration to the already-present navigation styles.
You'll have to add the navigation elements in FSE for this to show up.

This change also adds the drop shadow setting to match existing Blank Canvas style.

image

Additionally any core/navigation block that is in a <header> will receive "mobile" styling.
image
image

To support mobile styling quite a few additional custom styles have been added under custom.navigation.mobile.

Additionally javascript is loaded to support that menu by adding the button that opens the mobile menu. It's as small an amount of JavaScript as I could add and still be functional simply creating two buttons - open and close - and labeling them based on CSS variables which add/remove a show class to the menu block.

blank-canvas-blocks/assets/navigation.js Outdated Show resolved Hide resolved
blank-canvas-blocks/assets/navigation.js Outdated Show resolved Hide resolved
blank-canvas-blocks/assets/navigation.js Outdated Show resolved Hide resolved
blank-canvas-blocks/functions.php Outdated Show resolved Hide resolved
Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

Left a few minor comments, but in general this LGTM! I can live with the amount of code this adds to give us a temporary solution to navigation. We can iterate as needed when we implement a few child themes.

blank-canvas-blocks/assets/navigation.js Outdated Show resolved Hide resolved
blank-canvas-blocks/assets/navigation.js Outdated Show resolved Hide resolved
menuContainer.scrollTop = 0;
if( 0 === clickEvent.detail) {
// Menu was opened with keyboard, apply focus to close button.
menuContainer.querySelector('button:first-of-type')?.focus();
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, we can use optional chaining (?.) because we are not supporting IE, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, man, force of habit. Previously I ran all JS code through Babel to transpile it for IE.
None of this would work in IE though since it's so dependent on CSS Variables. Easy enough to add a null check instead though in case the CSS vars polyfill can do the trick here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored out the cool stuff.
Replaced with boring 'ol null checks.

blank-canvas-blocks/assets/navigation.js Outdated Show resolved Hide resolved
blank-canvas-blocks/assets/navigation.js Outdated Show resolved Hide resolved
pbking and others added 5 commits March 16, 2021 14:31
remove log

Co-authored-by: Jeff Ong <jeff.ong@automattic.com>
code formatting

Co-authored-by: Jeff Ong <jeff.ong@automattic.com>
Code nicificiation.

Co-authored-by: Jeff Ong <jeff.ong@automattic.com>
Code nicificiation.

Co-authored-by: Jeff Ong <jeff.ong@automattic.com>
@pbking pbking merged commit d5c656a into make/blank-canvas-blocks Mar 16, 2021
@pbking
Copy link
Contributor Author

pbking commented Mar 17, 2021

This (mobile) shim can be replaced when WordPress/gutenberg#22824 is completed.

@pbking pbking deleted the add/bcb-navigation branch March 25, 2021 13:34
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.

2 participants