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

Update HTML style guide with rule for camel casing attribute values. #11653

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented May 8, 2017

Per #11602 (comment):

We're using camel case pretty much everywhere (e.g. test subject selectors, even CSS class names), so I think it makes sense to be consistent by applying camel case to IDs too.

@jbudz
Copy link
Member

jbudz commented May 8, 2017

@cjcenizal mind if we get some more eyes on this or link to a previous discussion? The text LGTM but I'm against camel casing ids, and I can't find previous discussions. Most html ids I've seen use hyphens and I'm not sure why we'd change it.

@Bargs
Copy link
Contributor

Bargs commented May 8, 2017

It seems that any naming scheme relying on case sensitivity can lead to subtle bugs because html and css follow different case sensitivity rules. Different browsers also sometimes have different rules.

Quoting from the linked article:

The simplest way to mitigate any potential issues surrounding case sensitivity is to always use lowercase for everything in your markup and CSS, where possible. If that’s not possible, make sure the case you use is consistent between your CSS and your document markup.

@kobelb
Copy link
Contributor

kobelb commented May 8, 2017

I'm not necessarily advocating for this, but historically I've found myself using hypens for CSS classes and camelCasing for ids within HTML, but I think it's just a convention I integrated from working with Bootstrap.

data-test-subj="{{ 'clickMeButton-' + button.id }}"
>
{{ button.label }}
</button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per Slack convo w/ @archanid, we could expand this exception to use hyphens as a grouping mechanism among similar elements, e.g.

<section id="welcomePage-introSection">
  <!-- Content -->
</section>

<section id="welcomePage-aboutSection">
  <!-- Content -->
</section>

Ater thinking about this a bit I'm not sure how useful this would be, and I'd prefer to keep our naming as simple as possible. This still seems readable and understandable to me:

<section id="welcomePageIntroSection">
  <!-- Content -->
</section>

<section id="welcomePageAboutSection">
  <!-- Content -->
</section>

Copy link

Choose a reason for hiding this comment

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

Hm, I agree with you @cjcenizal. Simpler is easier to do and clearer to read.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@cjcenizal
Copy link
Contributor Author

@Bargs I looked into it and here is what I've found:

  1. The majority of our CSS uses elements or classes to define selectors.
  2. Of those which use attributes, the majority use just the attribute name (not the value), all of which use lower-case (and the attribute names are insensitive anyway).
  3. Of those which use attribute names and values, the attributes used are either type or class. The acceptable values for type are defined by the HTML spec, and the values for class are case sensitive.
  4. We have 31 instances of selectors using IDs, with a mix of camel-case and kebab-case. Styling IDs is a bad practice, so we should assume that we'll remove these selectors at some point, and avoid adding new ones in the future.

Given all of these, the only situations where I can see the casing being a problem in our CSS is when we use a [class=${something}] attribute as part of a selector. This seems to be a very rare edge case, since normally you'd just write .something instead. Based on this, I think that casing problems will be very rare events during development.

@Bargs
Copy link
Contributor

Bargs commented Jun 8, 2017

@cjcenizal Sounds reasonable, thanks for doing the additional research. Just to cover our bases, how would you feel about adding a note that casing in html and css should always match, even if one happens to be case insensitive?

@cjcenizal cjcenizal force-pushed the chore/html-style-guide-attribute-camel-case branch from 5740154 to 9135375 Compare June 8, 2017 19:25
@cjcenizal
Copy link
Contributor Author

@Bargs Updated, would you mind taking another look?

@cjcenizal cjcenizal force-pushed the chore/html-style-guide-attribute-camel-case branch from 9135375 to cb567f4 Compare June 8, 2017 19:27
@cjcenizal cjcenizal force-pushed the chore/html-style-guide-attribute-camel-case branch from cb567f4 to 28a8ab9 Compare June 8, 2017 19:28
@Bargs
Copy link
Contributor

Bargs commented Jun 8, 2017

LGTM!

@cjcenizal cjcenizal merged commit ed8951a into elastic:master Jun 8, 2017
@cjcenizal cjcenizal deleted the chore/html-style-guide-attribute-camel-case branch June 8, 2017 20:01
@cjcenizal cjcenizal removed the v5.6.0 label Jun 8, 2017
@jimgoodwin jimgoodwin added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Team:Operations Team label for Operations Team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:fix and removed Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Team:Operations Team label for Operations Team labels Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore release_note:fix Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.0.0-beta1 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants