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

[docs] Add css style guide section on open/closed principle #12276

Conversation

weltenwort
Copy link
Member

As discussed in #12197, this PR adds some links to external documentation and a section on the open/closed principle to the css style guide.

@weltenwort weltenwort force-pushed the documentation/css-style-guide-clean-code-open-close branch from f8da9ac to 4fa3418 Compare June 12, 2017 12:57
@weltenwort
Copy link
Member Author

cc @PopradiArpad

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.

This is outstanding, Felix! Thanks for adding this!


When working with the browser default styles or third-party stylesheets the
selectors are sometimes overly broad so that they have to be re-used in order
to integrate the styles into the project.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by re-using a selector so that the styles can be integrated into the project? Are there examples of this in the codebase?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of more generic selectors like a or input[type="text"], which are often styled by third-party libraries without care. Or take a widget library like ui-select, which does not allow the developer to specify his own classes. You're forced to modify existing classes provided by those libraries.

Copy link
Contributor

@cjcenizal cjcenizal Jun 13, 2017

Choose a reason for hiding this comment

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

Oh that makes a lot of sense. Could we reword this part? I usually think of this in terms of overriding selectors which we don't control. My suggestion:

We can't control the selectors introduced by third-party stylesheets and
these selectors may not adhere to our styleguide, e.g. `a` or `input[type="text"]`.
In these cases, we're forced to duplicate these selectors within our own
stylesheets and override those styles to control their look and feel.

When this happens, it's important to add comments which make it clear why these
selectors exist and which third-party dependencies they override. We should
also define these selectors in files which refer back to the dependency,
e.g. a file named `ui_select_overrides.less` will contain styles overriding those
introduced by the `ui-select` dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

absolutely, thanks for the improvement!

@rhoboat rhoboat self-requested a review June 14, 2017 12:42
Copy link

@rhoboat rhoboat left a comment

Choose a reason for hiding this comment

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

Great work. Please see my few comments.

</div>
```

To avoid the combinatorial explosion of classes with such groupings and its
Copy link

Choose a reason for hiding this comment

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

its reads weirdly here. Should it be their? Is it referring a block?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 it is referring to groupings, so their would be correct

modifiers, it is permissible to nest a block's selector under another block's
selector if:

* The relationship is a if a very narrow `element` to `elementGroup` kind that
Copy link

Choose a reason for hiding this comment

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

Whoa, what happened with this sentence?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, fixed


#### Exception: Normalization/Reset

We can not control the selectors introduced by third-party stylesheets and
Copy link

Choose a reason for hiding this comment

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

Can we please use cannot? It's a word. 🍡

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, TIL 😉

@weltenwort
Copy link
Member Author

@archanid I hope to have addressed your remarks. Could you take another look, please?

@weltenwort weltenwort removed the review label Aug 7, 2017
@weltenwort weltenwort merged commit 47db80d into elastic:master Aug 7, 2017
@cjcenizal
Copy link
Contributor

@weltenwort Could you please backport this to 6.x?

@weltenwort
Copy link
Member Author

Sure. What is the criterion for backporting style guide changes?

@cjcenizal
Copy link
Contributor

Anything which isn't a BWC change should go back into 6.x (so that branch and master will be near-identical). Now that I think about it, I think @epixa would like us to backport style guide changes all the way back to 6.0, too.

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

Successfully merging this pull request may close these issues.

3 participants