-
Notifications
You must be signed in to change notification settings - Fork 608
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
STENCIL-2558 - Update menu depth in navigation #939
Conversation
Autotagging @mcampa @bc-miko-ademagic @davidchin |
LGTM 👍 |
config.json
Outdated
@@ -596,4 +597,4 @@ | |||
} | |||
} | |||
] | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, this always creeps back out when making changes in theme editor, maybe we can adjust stencil cli to add a nl here...
schema.json
Outdated
"name": "Categories", | ||
"settings": [ | ||
{ | ||
"type": "checkbox", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to a dropdown or radios? The idea is to have multiple designs for the navigation, if in the future we add a new style we can add a line to the dropdown.
So, instead of a boolean, we should use a string setting with 2 possible values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done - it's a dropdown with multiple designs now
@@ -2,15 +2,26 @@ | |||
<div class="navPages-quickSearch"> | |||
{{> components/common/quick-search}} | |||
</div> | |||
<ul class="navPages-list"> | |||
<ul class="navPages-list{{#if theme_settings.menu_display_depth_max}} navPages-list-depth-max{{/if}}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more readable if we have something like:
{{#if theme_settings.navigation_design '==' 'dropdown'}}
{{> components/common/navigation-dropdown}}
{{/if}}
{{#if theme_settings.navigation_design '==' 'columns'}}
{{> components/common/navigation-columns}}
{{/if}}
This way it would be easier to add a third design in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's been changed - navigation_design can be 'simple' or 'alternate' where 'simple' is the default, existing design.
{{> components/common/navigation-dropdown}} | ||
{{else}} | ||
<a class="navPages-action" href="{{url}}">{{name}}</a> | ||
{{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can configure you editor to always add a new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried configuring my editor to remove it but it's saved by stencil-cli and it removes the nl. i think it's the jsonlint module: https://github.com/bigcommerce/stencil-cli/blob/master/lib/json-lint.js#L8. it gets rid of the nl. should we add it back when writing to file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes you should :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, yes, it's been added to navigation-list-alternate.html. was mixing up config.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -53,9 +56,14 @@ export class MobileMenuToggle { | |||
return this.$menu.hasClass('is-open'); | |||
} | |||
|
|||
get subMenuIsOpen() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSubMenuOpen
🍹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we just use it in once place. Can we just add the check directly instead of a separate method ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, the code's been moved directly inside the calling method
LGTM 💚 |
WHAT
WHY
ping @bigcommerce/stencil-team, @bc-ong