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

[Methodology, questions] Loose coupling, soft encapsulation #134

Open
dryoma opened this issue Apr 30, 2016 · 14 comments
Open

[Methodology, questions] Loose coupling, soft encapsulation #134

dryoma opened this issue Apr 30, 2016 · 14 comments
Labels

Comments

@dryoma
Copy link

dryoma commented Apr 30, 2016

Whilie thinking over and discussing SUIT design principles I found that I might not understand some moments fully.


Components should not directly modify the presentation or behaviour of their dependencies

Say we have a Header, whose children must be styled in some way (like, be floated or flexed, etc.). Some of those children are components themselves. There are three approaches one could think of:

a) apply the needed styles to .Header-inner and add this class to inner components:

.Header-inner { /* styles for header childern */ }

<div class="Header">
    <div class="Header-inner">...</div>
    <ul class="Menu Header-inner">
        <li class="Menu-item">...</li>
    </ul>
</div>

b) apply the needed styles to .Header-inner, but place the inner components inside them:

.Header-inner { /* styles for header childern */ }

<div class="Header">
    <div class="Header-inner">...</div>
    <div class="Header-inner">
        <ul class="Menu">
            <li class="Menu-item"></li>
        </ul>
    </div>
</div>

c) Apply the needed styles to .Header-inner and to the other components' modifiers And place those on the same level:

.Header-inner { /* styles for header childern */ }
.Menu--floated { /* includes styles for header children */ }

<div class="Header">
    <div class="Header-inner"></div>
    <ul class="Menu Menu--floated">
        <li class="Menu-item"></li>
    </ul>
</div>

a) seems the worst as it breaches the "your component should not leak styles into the HTML tree fragments of other components" principle. So I would pick b) or c).


  1. Yet, all three examples above seem to breach the

a component's HTML should not be directly included in the HTML for another component.

This can be confusing. I really doubt that the principle discourages from having some components for minor or major layouts (like .Header, .Menu), while the others for simple building bricks (.Button). So this could mean that its ok to place components inside other components, but a component should not rely on that some other components would be inside its HTML? Like, not having such rules as:

.Header .Menu { ... }
.Menu .Button { ... }

Could the authors please elaborate on these moments?

@simonsmith
Copy link
Member

The docs do go into some detail on the situation you describe - Styling dependencies.

There is also this great post by Philip Walton - Extending Styles. I've had success with option 4 when using postcss-extend.

@giuseppeg
Copy link
Member

Regarding your first question, yep wrapping (b) is in my opinion the best way to go.
You make every component full width, marginless, avoid layout rules and let the context decide its size/layout etc.

<style>
.Button {
  margin: 0;
  width: 100%;
}
.Header-button {
  width: 20%;
  float: right;
}
</style>

<header class="Header">
  <h1 class="Header-title">Hey bud</h1>
  <span class="Header-button">
     <button type="button" class="Button">Do Something</button>
  </span>
</header>

I wrote a little thing around this topic with focus on margins.

Regarding question no. 2, I don't think that one should avoid components nesting (in html):

a component's HTML should not be directly included in the HTML for another component.

Things that one usually wants to control from the parent are font-size and colors. In this case one could just use attached classes or break the rules a bit and take advantage of cascading like here https://www.youtube.com/watch?v=jPOBVaomzLE (never tried this at scale though).

Otherwise maybe postcss-apply is preferable to extend.
It is future-proof and more in line with the composition feature of CSS Modules.

@dryoma
Copy link
Author

dryoma commented May 4, 2016

Hey guys, thanks for your thoughts!

The docs do go into some detail on the situation you describe

So, SUIT basically allows 2) and 3) ?

There is also this great post by Philip Walton - Extending Styles

My opinion on extending: from lots of code I've seen I concluded that extend-ing is a dangerous path :) Importing inside components is as well. Would really like to avoid those.

I wrote a little thing around this topic with focus on margins.

Oh, that one is a good read as well, thanks.

Regarding question no. 2, I don't think that one should avoid components nesting (in html):

I agree. The thing is, SUIT docs state that clearly and firmly: don't place components' HTML into other components' HTML, i.e. no nesting. Our team decides on the codestyle (and ideally a methodology) to adopt, so explaining every person new to SUIT that "yeah, they say it but they don't mean it" would be a strange and confusing. Maybe the docs need an update on this?

@simonsmith
Copy link
Member

The documentation says:

a component's HTML should not be directly included in the HTML for another component.

That means that a Header component shouldn't have a Logo component in it by default, but it can of course accept components as children. This is why it's better to style dependencies with a Header-wrapLogo approach or by passing in a class from above. It keeps the parent agnostic about the child.

There are two examples in the docs that show this:

<article class="Excerpt u-cf">
  {{! other implementation details }}

  <read-button class="Excerpt-button">Read more</read-button>

  <div class="Excerpt-wrapButton">
    <read-button>Read more</read-button>
  </div>
</article>

In both cases the read-button is a separate component and doesn't exist inside Excerpt. This means other components could be added there, and also read-button can be reused elsewhere as it's implementation is separate.

If the documentation is not clear enough then we could definitely improve it.

@dryoma
Copy link
Author

dryoma commented May 7, 2016

If the documentation is not clear enough then we could definitely improve it.

If possible, I think that would be great. I'd also propose to make read-button in the example above a little more elaborate (<a class="Button"> or something like that, to make it more clear that it is a component). as well as some comments on this naming choice (.u-cf) considering this part of the styleguide and the example of bad naming.

In fact, it's not just my personal vision; all these are things my teammates pointed at as issues/contradictions (and I'd really love our team to adopt SUIT).

@simonsmith simonsmith added the docs label May 7, 2016
@simonsmith
Copy link
Member

By expanding the read-button into normal HTML it turns it into the very thing we're trying to avoid, which is including the HTML in another component. The current example helps reenforce the idea that it's a separate component being placed in the Excerpt. It's exactly the same as React.createElement which most people are familiar with. Perhaps React example code might be easier to understand here.

Your example of the naming is covered elsewhere in the documentation:

Any classes with terse names, e.g., u-cf and u-nbfc, are either particularly abstract or very commonly used utilities with otherwise excessively long names. For example, the u-cf utility is used to "contain floats" without clipping any overflow; the u-nbfc utility is used to create a "new block formatting context".

@dryoma
Copy link
Author

dryoma commented May 9, 2016

Ah, thanks, I read that part too long ago so totally missed it now.

As for the read-button - sure, I get it. Just thought it would be clearer that way.

@dryoma
Copy link
Author

dryoma commented Mar 20, 2017

Hey everyone,

I came across a case I'm unsure how to deal with according to SUIT (and whoa! - there is a convenient thread still not shot in the head :) ). Let's say, there is a component inside another component; and we need to be able to control the looks of the child component's elements with the parent component's modifiers/class states. For example:

<form class="Form">
  ...
  <button class="Button"><i class="Button-icon"></i> some text </button>
</form>

here we might want to make the Button-icon to be different on .Form--question, to spin on .Form.is-loading, ect.

  1. The most obvious option,
.Form.is-loading .Button-icon

is simpler and easier to implement (for a person building the markup, as he won't omit necessary details by mistake). But it introduces undesired cross-component dependencies.

  1. <button class="Button Form-btn"><i class="Button-icon Form-btnIcon></i></button> seems too complex and mixing a component and its element with elements of a different component. And more error prone as forgetting to add any of those two extra classes would result in the functionality getting broken.

So what would be the correct way of doing that with SUIT, assuming we have control over how these components are built? How do you deal with such situations?

@giuseppeg
Copy link
Member

There is a section in the docs https://github.com/suitcss/suit/blob/master/doc/components.md#styling-dependencies

and an interesting blog post on this topic http://simurai.com/blog/2015/05/11/nesting-components

Tbh I don't know the answer yet though :) If you are only targeting evergreen browsers and can use custom properties then maybe you have better options

@dryoma
Copy link
Author

dryoma commented Mar 21, 2017

Hey @giuseppeg

Yep, I have read the docs. More than once actually :) Still, there's no clear answer to the question above there. As for the blog post - It basically goes into more detail on the options from the first message here, which is not what I'm scratching my head at now :)

I'd be ok with a modifier for Button (the 3rd option, yeah), if only it could solve the state class issue. 'Cause I really wouldn't want to add a state class not just to the Form, but for the Button, and any other standalone component inside that Form.

@simonsmith
Copy link
Member

It feels like the cleanest seperation is to add .is-loading to the root of the Button. That way the Form has no knowledge about the internals of Button. It's similar approach to passing a prop to a component.

@oleersoy
Copy link
Contributor

Say we have a Header, whose children must be styled in some way (like, be floated or flexed, etc.). Some of those children are components themselves. There are three approaches one could think of:

I would not start with the Header as part of the design scope. Just start with what you want to design. Whether it's later going to go in a header, footer, main, etc. is irrelevant. Design your components and utilties to be standalone and render the same regardless of where they are.

You can also make utilities that can be applied to the component later when you are done prototyping out the design, as I have done here:

superfly-css-component-test

This way you can prototype out your component design using utilities and just apply them when you are done. This also allows you to change the core component design which is helpful when you don't need a modifier like Button Button--orange. Instead the default Button is orange.

@dryoma
Copy link
Author

dryoma commented Mar 27, 2017

@simonsmith

It feels like the cleanest seperation is to add .is-loading to the root of the Button.

Yeah, it is. Just hoped there is a simpler way. Because passing props might be not that trivial in non-react environments. But, oh well, will have to stick to that option. Thanks for discussing that.

@oleersoy thanks for your thoughts. A legit pattern, surely, but only for part of cases. That simply won't work if one already has a page design and needs to implement it. Or if Header just has to be a component, which it usually is. In any case, both questions were about things different from what you were describing.

@oleersoy
Copy link
Contributor

@dryoma you are much better of leaving cross component state in the javascript domain.

@simonsmith simonsmith added suit and removed suit labels Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants