suitcss / suit

Style tools for UI components
http://suitcss.github.io/
3.8k stars 225 forks source link

[Methodology] Inheritance #139

Closed danturu closed 7 years ago

danturu commented 7 years ago

I'm curious what is the correct way to apply same styles for the p and a tag in the following React's components:

<AuthLayout>
  <Signin />
</AuthLayout>
<AuthLayout>
  <Signup />
</AuthLayout>

Signin:

<div className="Signin">
  <div className="Signin-header>
    <p>Text and <a href="/abc">a link</a></p>
  </div>

  <Form>
     ...
  </Form>
</div>

Signup:

<div className="Signup">
  <Form>
     ...
  </Form>

  <div className="Signup-footer>
    <p>Text and <a href="/abc">a link</a></p>
  </div>
</div>
  1. Should I create the Paragraph and Link components just for the styles? In the rest app's part the styles for the a and p tags are completely different and modifiers doesn't work here. p and a almost look like AuthLayout's descendants.

  2. Is it OK to have component-less styles SigninText.css and include it in the Signin and Signup components:

SigninText.css:

SigninText-link {
  ...
}

SigninText-text {
  ...
}

... and then:

Signin:

import '../SigninText.css';
import '../Signin.css';

<div className="Signin">
  <div className="SigninText Signin-header> // BEM's composition 
    <p className="SigninText-text">Text and <a className="SigninText-link"href="/abc">a link</a></p>
  </div>

  <Form>
     ...
  </Form>
</div>

Signup:

import '../SigninText.css';
import '../Signup.css';

<div className="Signup">
  <Form>
     ...
  </Form>

  <div className="SigninText Signup-footer> // BEM's composition
    <p className="SigninText-text">Text and <a className="SigninText-link" href="/abc">a link</a></p>
  </div>
</div>
briandrum commented 7 years ago

Would this work?

CSS:

.AuthLayout {
  …
}

.AuthLayout--signIn {
  …
}

.AuthLayout--signUp {
  …
}

.AuthLayout-header {
  …
}

.AuthLayout-footer {
  …
}

.AuthLayout-text {
  …
}

.AuthLayout-link {
  …
}

Signin:

<div className="AuthLayout AuthLayout-signIn">
  <div className="AuthLayout-header">
    <p className="AuthLayout-text">Text and <a className="AuthLayout-link" href="/abc">a link</a></p>
  </div>

  <Form>
     …
  </Form>
</div>

Signup:

<div className="AuthLayout AuthLayout-signUp">
  <Form>
     …
  </Form>

  <div className="AuthLayout-header">
    <p className="AuthLayout-text">Text and <a className="AuthLayout-link" href="/abc">a link</a></p>
  </div>
</div>
simonsmith commented 7 years ago

From the documentation:

Avoid coupling or entangling components, even if that means the code is not as DRY as you think it should be.

I can't imagine the p and a elements in question have a great deal of styles so for the sake of simplicity I'd take a small hit of duplication and just create .Signup-text and Signin-text in the respective component files.

If duplication was still a concern you could consider breaking out some of the declarations into utilities that be used across any component. For example:

.Signin-text {
  color: #444;
  display: block;
}

/* becomes */

<p class="u-colorMuted u-block">Text</p>

Reuse is always something to keep in mind, but don't let it trump simpler code and easier understanding.

danturu commented 7 years ago

@briandrum It would work, but AuthLayout is a router-level layout and it's implemented something like:

const AuthLayout = ({ children }) => (
  <div className="AuthLayout">
    <Wordmark className="AuthLayout-logo" />
    {children}
  </div>
)

So the router wraps components internally. Of course I could create another one intermediate layout like you suggested, but I'm not sure that it's a right way.

@simonsmith Thank you! I've been thinking about utilities, but yep I agree that duplication is a better way here.

I came from BEM so does Suit avoid https://en.bem.info/forum/4 and especially https://en.bem.info/forum/4/#comment-111734861?

Closed.

simonsmith commented 7 years ago

@rosendi If I understand those posts correctly you're talking about using multiple class names on a component?

danturu commented 7 years ago

@simonsmith yes. Let's say we have a component I mentioned above:

const AuthLayout = ({ children }) => (
  <div className="AuthLayout">
    <Wordmark className="AuthLayout-logo" />
    {children}
  </div>
)

In order to change the geometry (margins) of wordmark I add AuthLayout-logo. It renders into <div class="Wordmark AuthLayout-logo>.

simonsmith commented 7 years ago

That is one approach you can take (and it's listed in the documentation) but I tend to avoid it where possible. It can be too easy to have specificity issues if your source order is not precise.

Instead I prefer a wrapX approach:

const AuthLayout = ({ children }) => (
  <div className="AuthLayout">
    <div className="AuthLayout-wrapWordmark">
      <Wordmark />
    </div>
    {children}
  </div>
)

Providing your components do not specify their own margins then you're now able to control the layout. This is exactly how Grid works:

<div class="Grid">
  <div class="Grid-cell">
      <Component />
    </div>
</div>

You are free of course to use more generalised naming such as wrapItem or wrapChild if you expect the children to vary.

Where 'mixing' class names can be useful is with utilities as these target very specific styles and are designed to be used together. Their use of !important means that component styles will not get in the way. For example:

<header class="Header u-sm-flex u-sm-flexJustifyBetween u-sm-flexAlignItemsBaseline">
  <div class="Header-wrapLogo">
    <Logo />
  </div>
  <nav class="Header-wrapNav">
    <Nav />
  </nav>
</header>

Here the utils-flex classes control the layout of the child components and the wrapX elements allow fine tuning of margins etc, particularly across breakpoints. Meanwhile Logo and Nav can be reused elsewhere with little bother.

Mixing classes have their place but it's something I tend to reach for last if possible.

Here is a good article that touches on that topic - https://philipwalton.com/articles/extending-styles/

Hope that helps.

danturu commented 7 years ago

@simonsmith thank you for your answer!