-
Notifications
You must be signed in to change notification settings - Fork 541
feat(menu): Add BEM classes #297
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
Conversation
You know the drill. - Updated BEM classes, including root, header, footer - Updated docs BREAKING CHANGE: The default template now has the count element inside the link, not outside.
@@ -47,7 +47,7 @@ function menu({ | |||
var containerNode = utils.getContainerNode(container); | |||
var usage = 'Usage: menu({container, facetName, [sortBy, limit, cssClasses.{root,list,item}, templates.{header,item,footer}, transformData, hideWhenResults]})'; | |||
|
|||
if (container === null || facetName === null) { | |||
if (!container || !facetName) { |
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.
Why? :) Was there an edge case that we didn't support in this case?
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.
Nope, it's a line that had been forgotten in a previous refactoring I guess.
We used to pass default parameters as container = null, facetName = null
and we then decided to simply pass container, facetName
with no value and test if they were falsy.
Here we had done only half the job so I changed the second part :)
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.
Oh we decided that? Cool then. :)
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 was more an oral decision that anything written down actually ;)
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.
AH :) Be careful if we are to have a global team anytime :)
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.
Yeah I know, we'll have to have a place to write those styleguide/standards somewhere soon. Just a wiki page would do.
LGTM! 👍 |
You know the drill.
BREAKING CHANGE: The default template now has the count element inside
the link, not outside.