Class naming convention #795

Closed
dstorey opened this Issue Jun 12, 2012 · 30 comments

Projects

None yet

6 participants

@dstorey
dstorey commented Jun 12, 2012

(was: text-slider class name inconsistent)

Since the Skeleton theme update, all the class names for the base element of a component use a consistent naming scheme of "montage-componentName". E.g. "montage-tokenField".

The textSlider component uses "montage-text-slider". This should be updated to "montage-textSlider". Makes styling the components more predicable if the names are consistent.

@Stuk Stuk was assigned Jun 12, 2012
@Stuk
Member
Stuk commented Jun 13, 2012

There doesn't actually seem to be that much consistency: (stripping out components without dashes)

./input-range.reel/input-range.html: class="montage-inputRange"
./loading-panel.reel/loading-panel.html: class="montage-loading-panel"
./popup/alert.reel/alert.html: class="montage-alert-dialog"
./popup/confirm.reel/confirm.html: class="montage-confirm-dialog"
./popup/popup.reel/popup.html: class="montage-invisible montage-popup-container"
./rich-text-editor/overlays/rich-text-linkpopup.reel/rich-text-linkpopup.html: class="montage-link-popup"
./rich-text-editor/overlays/rich-text-resizer.reel/rich-text-resizer.html: class="montage-resizer-container"
./rich-text-editor/rich-text-editor.reel/rich-text-editor.html: class="montage-editor"
./scroll-bars.reel/scroll-bars.html: class="montage-scroll-bars"
./text-slider.reel/text-slider.html: class="montage-text-slider"
./token-field/token-field.reel/token-field.html: class="montage-tokenField"
./video-player.reel/video-player.html: class="montage-video-player"

tokenField and inputRange are the only components following this convention. Personally I would prefer the dash-form (montage-token-field) for two reasons: 1) everywhere else that something begins with a lowercase letter it is dashed (first uppercased means camel case) 2) most of our CSS uses dashes and not camel case.

@dstorey I'm assuming you don't really care which way it goes as long as it's consistent? (which I agree on) Do you have any opinions @kishores ?

@dstorey
dstorey commented Jun 13, 2012

@stuk they suppose to be consistent. All the regular components that have a native equivalent *and some of the others, such as tokenField) have been updated. I have a table in http://tetsubo.org/2012/06/introducing-the-new-default-skeleton-theme/

I don't have an opinion either way if it is montage-token-field or montage-tokenField, as long as it is consistent.

@kishores
Contributor

Agree with @Stuk. I think montage-input-text is consistent with the naming of the reel.

@mczepiel
Member

Yeah, I think we'd decided that filenames/attributes would be all lowercase and dash delimited; this follows the css property names to JavaScript equivalence.

@simurai
Contributor
simurai commented Jun 13, 2012

@Stuk Those other components don't have a consistant naming because they were created "pre-Skeleton" and actually still should be updated as well.

So here is the reason to the camelCase classes in CSS: The goal was that when you look at a montage CSS class, you can already see how the markup is build. Therefore each child element is separated with a dash. As an example:

<div class="montage-inputRange">
    <div class="montage-inputRange-thumb"></div>
</div>

So if you first customize only the thumb in your stylesheet with .montage-inputRange-thumb {...} and later come back to also style the track you would know that its parent would be .montage-inputRange {...}. With montage-input-range, it would break that convention and you would have to check the inspector or the source.

If everybody is against the camelCase, we could also use an underscore for child elements. A bit like this: https://gist.github.com/1309546 but only one. So it would be:

<div class="montage-input-range">
    <div class="montage-input-range_thumb"></div>
</div>
@Stuk
Member
Stuk commented Jun 13, 2012

Ahhh, interesting, ok. Ignore the above pull request, I didn't get an email with your comment and have only just seen it.

@simurai
Contributor
simurai commented Jun 13, 2012

@Stuk Oops, sorry.. commented too late. Well, I'm not married to the current version and open for alternatives. ;-) Using dashes to split up double names would be nice so that it's the same like the reels. But then it would also be nice to somehow differentiate child elements in another way. Like using underscores for those.

@Stuk
Member
Stuk commented Jun 13, 2012

Yep, it's a good idea. I'm wondering about states: for example in text-slider I currently have the class montage-text-slider-editing. Do you think this should be montage-text-slider_editing?

@simurai
Contributor
simurai commented Jun 13, 2012

How about double dash for states? Same as https://gist.github.com/1309546 So it would be montage-text-slider--editing?

And the child elements with underscore so they don't conflict:

<span data-montage-id="text-slider" class="montage-text-slider" tabindex="0">
    <input type="text" data-montage-id="input" class="montage-text-slider_input"/>
    <span data-montage-id="value" class="montage-text-slider_value"></span>
    <span data-montage-id="units" class="montage-text-slider_label"></span>
</span>

And also double--dash for variations.. like if there would be different kind of text slider: montage-text-slider--large?

@Stuk
Member
Stuk commented Jun 13, 2012

I really like this idea. I want to throw the montage-google stuff into the mix to see how that would work

<div data-montage-id="container" class="montage-google-youtube-channel montage-google-youtube-channel--hide">
    <img src="" alt="" data-montage-id="imageA" class="montage-google-youtube-channel_thumbnail">
    <img src="" alt="" data-montage-id="imageB" class="montage-google-youtube-channel_thumbnail">
    <img src="" alt="" data-montage-id="imageC" class="montage-google-youtube-channel_thumbnail">

    <div data-montage-id="popup" class="montage-google-youtube-channel_popup">
        <button data-montage-id="close" class="montage-google-youtube-channel_close">×</button>
        <div data-montage-id="player" class="montage-google-youtube-channel_player"></div>
    </div>
</div>

This almost makes me think that montage_text-slider and montage-google_youtube-channel would be better... except that it looks ugly and is annoying to type.

@simurai
Contributor
simurai commented Jun 14, 2012

What is the montage-google stuff? You mean if Montage needs to be Google branded? Ohh.. dear! Please no longer namespaces..!!! ;-) montage- feels already a bit too long. Maybe then we could use mg-?

Hey, have you tried double-clicking those names? Dashes separate the selection, underscores get combined.. so this brings me to this: montage-text_slider-input. Would be quicker to select just the parts that actually belong together. The only problem.. Underscores makes them visually more separated. ;-( I guess that's why montage-textSlider-input works nice in that regard.

Stupid question and I guess it was already dismissed earlier, but could the reels be camelCase? Like montage/ui/textSlider.reel. Or would that case some problems with servers or so?

@Stuk
Member
Stuk commented Jun 14, 2012

montage-google contains several Google APIs wrapped up into delicious components: https://github.com/Motorola-Mobility/montage-google.

Ahh yeah, the double click thing is an interesting usability problem... will need to think about this a bit more

@mczepiel
Member

We're avoiding camelCase in reels to avoid server case-sensitivity issues. The translation from class name to reel name is to dash-delimit the camelCase; please don't use underscores to do the same thing in the CSS classes.

Underscores would be fine between other parts of the classname montage_text-slider_input but it's a pain to type.

Is there a huge problem in making the parts less obvious by just using dashes everywhere?

Otherwise, could we use descendent selectors to avoid overly long/specific classnames?
e.g. .montage-text-slider and .montage-input styleable with the selector .montage-text-slider .montage-input

@simurai
Contributor
simurai commented Jun 14, 2012

Ya, using descendent selectors like .montage-text-slider .montage-input would be an option, but it has a higher risk of class collision. We couldn't create an input.reel because that would also use the .montage-input class. Also single selectors perform a bit better, although the difference is too small to worry about.

@simurai
Contributor
simurai commented Jun 14, 2012

Ok, all options together:

  1. .montage-textSlider-input not same as reel name
  2. .montage-text-slider-input markup not distinguable, not double-click friendly
  3. .montage_text-slider_input harder to type, not double-click friendly
  4. .montage--text-slider--input not double-click friendly
  5. .montage-text_slider-input Mike says no! + not same as reel name, looks visually more disconnected
  6. .montage-text-slider .montage-input risk of name collision

Votes are on!

Mine goes to Nr. 1 because the only disadvantage is that it's not the same as its reel name, but has the benefits of being double-click friendly and has a distinguable markup hierarchy.

@Stuk
Member
Stuk commented Jun 19, 2012

We could go with 1, but properly CamelCase it: .montage-TextSlider-input. Components would be CamelCased, elements would be lowercase, states could be appended with --

.montage-TextSlider
.montage-TextSlider-input
.montage-TextSlider--editing

/* if TextSlider had a DynamicText descendent that *needs*
to be styled differently from the regular DynamicText... */
.montage-TextSlider-DynamicText 
/* ...otherwise use descendent selectors */
.montage-TextSlider .montage-DynamicText
@kishores
Contributor

+1. I like .montage-DynamicText- better than .montage-dynamicText-.

@Stuk
Member
Stuk commented Jun 19, 2012

To get some input from Ninja peeps @ericguzman @dhg637 @jreid01

@simurai
Contributor
simurai commented Jun 20, 2012

Yeah, I kinda like that only components use CamelCase (then they match JS) but not it's children. Gives them more weight. The only question is what if the children also "need" multiple names? Like .montage-TextSlider-inputwithmultiplenames. We could try to avoid that, but I'm sure sooner or later it's gonna be limiting in name choosing and the meaning becomes less clear. Should it be .montage-TextSlider-InputWithMultipleNames?

But then should we use CamelCase on all children so that the multi name ones don't look like a component?

.montage-TextSlider-Input
.montage-TextSlider-Label
.montage-TextSlider-ChildWithMultipleNames
@Stuk
Member
Stuk commented Jun 20, 2012

Do you mean if the component has two child elements of the same type?

/* hypothetical component */
.montage-NameInput-inputFirstName
.montage-NameInput-inputLastName
@simurai
Contributor
simurai commented Jun 21, 2012

Ahh.. sorry, I actually meant multiple words. You know if you wanna make it more clear and instead of just using a single word "input" like you did in the text slider .montage-TextSlider-input you want to name it "input with pink ears". Then it also needs to be CamelCase otherwise it would be .montage-TextSlider-inputwithpinkears and unreadable.

So to be constant, all children would be CamelCase, no just the ones that have multiple words in their name.

.montage-TextSlider-Input
.montage-TextSlider-InputWithPinkEars
@Stuk
Member
Stuk commented Jun 22, 2012

I would prefer having CamelCase for child components and camelCase for child elements, to make the distinction clear. In the most common case the child elements will be just one word, and so will be all lowercase.

@simurai
Contributor
simurai commented Jun 23, 2012

OK.. deal! I put the whole thing into the Skeleton Spec.

@Stuk
Member
Stuk commented Jun 23, 2012

Awesome. I'll create a pull request changing all the current classes, unless you'd rather do it?

@simurai
Contributor
simurai commented Jun 25, 2012

Sure, you can go ahead. And I'll give it a "hawk eye". ;-)

This was referenced Jun 27, 2012
@dstorey
dstorey commented Jul 2, 2012

Should there be a google doc somewhere explaining the agreed convention, so it doesn't get lost in this bug report?

@Stuk
Member
Stuk commented Jul 2, 2012

Yes :)

@simurai
Contributor
simurai commented Jul 3, 2012

@dstorey Yes, it's in the Skeleton Spec, but might be good to have a separate "Naming Convention" doc?

@Stuk Stuk referenced this issue Jul 12, 2012
Merged

Class names update #932

@Stuk Stuk closed this Apr 26, 2013
@MoOx MoOx referenced this issue in sapegin/grunt-webfont Oct 20, 2013
Closed

Add SUIT syntax template #55

@kevinSuttle

I just saw this example in the CSS naming conventions.

<button data-montage-id="button" class="digit-Button">

How is the data- attribute being leveraged, and how does this casing convention handle find/replace in searching?

@simurai
Contributor
simurai commented Dec 14, 2014

@kevinSuttle The data- attribute doesn't get used as a CSS selector. It's only for JS. Similar like the js- convention, but more separated as an attribute. In the CSS naming conventions, it probably should just say:

<button class="digit-Button">

to avoid confusion.

Edit: Ok, it's now removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment