Skip to content

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

Merged
merged 1 commit into from
Oct 20, 2015
Merged

feat(menu): Add BEM classes #297

merged 1 commit into from
Oct 20, 2015

Conversation

pixelastic
Copy link
Contributor

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.

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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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 :)

Copy link
Contributor

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. :)

Copy link
Contributor Author

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 ;)

Copy link
Contributor

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 :)

Copy link
Contributor Author

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.

@bobylito
Copy link
Contributor

LGTM! 👍

bobylito added a commit that referenced this pull request Oct 20, 2015
@bobylito bobylito merged commit 508931b into develop Oct 20, 2015
@vvo vvo removed the in progress label Oct 20, 2015
@pixelastic pixelastic deleted the feat/bem-menu branch October 21, 2015 08:11
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