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

[K7] BEM mixin #12884

Merged
merged 5 commits into from
Jul 17, 2017
Merged

[K7] BEM mixin #12884

merged 5 commits into from
Jul 17, 2017

Conversation

snide
Copy link
Contributor

@snide snide commented Jul 14, 2017

Adds bemify to our mixins. Slight adjusted to be usable with our slanted rules, which are discussed in the styleguide.

@snide snide requested a review from cjcenizal July 14, 2017 23:55
@kimjoar
Copy link
Contributor

kimjoar commented Jul 15, 2017

Why bemify? Never heard of it, and doesn't seem like a highly depended upon in the community. Will it create a barrier for people on the team to contribute to the sass code?

@cjcenizal
Copy link
Contributor

@kjbekkelund These are valid concerns, thanks for bringing them up! Our goal is to make BEM as "automatic" as possible when people contribute SCSS. It's a lot easier for people to conceptually grok components vs children vs modifiers, but not always so easy to remember the exact syntax (especially if you don't spent a lot of time in CSS-land in general). Bemify is a pretty simple set of mixins for automatically generating the correct BEM syntax in our CSS classes. If it's as useful as we hope, then it should make it easier for people to contribute SCSS. If it turns out to be cumbersome or to introduce workflow problems (e.g. it becomes more difficult to grep for components) then we'll remove it or find another solution.

@@ -15,6 +15,10 @@ Our style guide is an extension of [Sass Guidelines by Hugo Giraudel](https://sa
The Sass Guidelines site recommends using RBG and HSL values to format colors, but we're using
hex values.

### Bemify for namespacing

We use the [bemify](https://github.com/franzheidl/bemify) for namespacing the sass into a BEM format. We use `component`, `child`, `modifier` and `state` as our mixin names. We've adjusted the plugin's state mixin so that you need to write out the full selector (`@include state('is-happening')`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an example to demonstrate how each mixin should be used? This should also help address some of @kjbekkelund's questions about why/what, if they occur to anyone else.

@kimjoar
Copy link
Contributor

kimjoar commented Jul 15, 2017

Thanks for explaining, @cjcenizal. I expect some examples and better descriptions in the readme will help much. It still "feels" weird, but it'll be much easier for me to see how it feels when we add more code

@snide
Copy link
Contributor Author

snide commented Jul 15, 2017

If we continue to have a large amount of people contributing CSS I'd like to use a mixin system to keep namespacing correct. It's obviously a pain point in our current system and there are very little downsides. I've used similar systems in the past

I'll add some examples.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

@kjbekkelund How does this example look to you?

@snide Thanks for adding it! I just had a couple small suggestions based on our current conventions.

#### Example

```
// Generates .kuiBtn
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the existing kuiButton class name for this example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

}

// Generates .kuiBtn.is-loading
@include state('is-loading') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're currently formatting our state classes as kuiButton-isLoading -- can we use that here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Will just need to adjust the mixin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Err, adjust my docs, not the mixin.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM! Our current state classes are a bit different than this (kuiButton-isLoading vs isLoading) but we can figure out a way to tweak these mixins later on.

@snide snide merged commit a240cf9 into elastic:k7-ui-framework Jul 17, 2017
@snide snide deleted the k7/bemmix branch July 17, 2017 16:20
cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Jul 19, 2017
Bemify mixin for handling namespacing in our sass.
cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Aug 16, 2017
Bemify mixin for handling namespacing in our sass.
cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Aug 26, 2017
Bemify mixin for handling namespacing in our sass.
cjcenizal pushed a commit that referenced this pull request Sep 19, 2017
Bemify mixin for handling namespacing in our sass.
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