Travix-International / travix-ui-kit

[DEPRECATED] Travix UI Components' repository.
https://travix-international.github.io/travix-ui-kit/
MIT License
8 stars 10 forks source link

Extending styling while using CSS Modules #178

Open fahad19 opened 7 years ago

fahad19 commented 7 years ago

Browsers and versions affected

n/a

Description

The problem

Possible solution

One way we can enable this while both maintaining CSS Modules and allowing developers to extend the UI-Kit Components in their Apps is by supporting a className prop in all UI-Kit components.

// my/custom/app/components/MyComponent.js
import React from 'react';
import { Button } from 'travix-ui-kit';

import styles from './styles.css';

export default function MyComponent(props) {
  return (
    <div>
      <h1>Hello World</h1>

      <Button className={styles.fancyButton}>
        Click me!
      </Button>
    </div>
  );
}

And the styles in a CSS Module can be:

/* my/custom/app/components/styles.css */
.fancyButton {
  color: tomato;
  letter-spacing: 500px;
}

Steps to reproduce

Expected results

Being able to extend the styling of UI-Kit components shouldn't require knowing the CSS class names used by the Components internally.

And developers should be allowed to do so without being able to modify the styling of UI-Kit elsewhere. Only their own Apps are affected by their own code.

Actual results

n/a

asci commented 7 years ago
  1. Introducing fancyButton leads to visual inconsistent in UI. If you need something like this probably:

    • you need to discuss this with designers and change design to contain button from UI kit
    • or you need to add a new variation to UI kit We also have propmods. But I found only 2 places in RWD repo we're actually using mods
  2. Is there a solution if you don't disable :global?

fahad19 commented 7 years ago
asci commented 7 years ago

example of fancyButton is typical and the idea applicable not only to fancyButton

mAiNiNfEcTiOn commented 7 years ago

@fahad19 is this the effective issue related to #153 ?

fahad19 commented 7 years ago

@mAiNiNfEcTiOn: they are separate.

Issue #153 is about using postcss-travix for handling the CSS bundling, so we can benefit from one single config everywhere.

This issue is a feature request for the Components (JS) in UI-Kit.

asci commented 7 years ago

so, any thoughts how can we prevent inconsistency of UI when we introduce classNames (instead of mods)?

fahad19 commented 7 years ago

enabling className prop would allow other developers to extend the styling of UI-Kit components, no doubt.

but with CSS Modules and :global disabled, we at least know that they can only mess up their own Apps, not someone else's. It will be contained.

if there happens to be inconsistencies, manual reviews would be needed.

I suggest establishing something like design guidelines for Travix, like how Ant Design does here for example: https://ant.design/docs/spec/introduce

Enable developers to do things. But guide them about what to do, and especially what not to do.

yurist38 commented 7 years ago

Here is the case: I'm working on seatmap widget right now. I have HTML structure built by amadeus script, that structure contains static CSS classes what I need to style. What if we allow using global css class names only inside of generated ones? So we will be sure that we are override/extend only content inside of our parent (widget/block).

mAiNiNfEcTiOn commented 7 years ago

I agree with @fahad19, exposing className as an attribute would be a good way to go.

Also, I would add that:

// this would be ok:

render() {
  const { className } = this.props;
  return (
    <div className={className}>
      bla
    </div>
  );
}

// or...

import SubComponent from './SubComponent'; // and internally this would behave as the previous example

/*...*/

render() {
  const { className } = this.props;
  return (
    <div>
      <SubComponent className={className} />
    </div>
  );
}

// this would NOT be ok

render() {
  const { className } = this.props;
  return (
    <div>
      <div className="other-stuff">
        <h3  className={className}>Bla</h3>
      </div>
    </div>
  );
}

If you have the need to have a className in an inner element generated by you, either extract it as a component to pass the attribute (which in this last case would make no sense to do an H3 component, so just do proper CSS :D)

asci commented 7 years ago

So, guys, you're not even considering that you may not need classNames at all :)? As I said we have only 2 places in our codebase where we use mods. And those cases could\should go to UI kit itself.

The idea is to have standard UI. If you need to customize it — add it to UI kit, so other developers will be able to reuse it. Or talk to designers and change the design to use components from the library. If even this is not the case — you may not need UI kit at all for your component.

Let's wait for @iwwwi before taking action on this

mAiNiNfEcTiOn commented 7 years ago

@asci sure... still some discussion never harmed anyone.

Can you elaborate more on the

you may not need classNames at all :)?

? Since we're discussing the possibility of using CSS Modules on UI Kit, using classNames would definitely be a need... And perhaps a replacement for BEM's methodology?

asci commented 7 years ago

@mAiNiNfEcTiOn using CSS Modules inside UI kit doesn't require to expose it to public API.

you may not need classNames at all

I tried to elaborate on this in my message Let's try from the other side — when do you expect to use classNames?