Open petermakowski opened 2 years ago
Thanks for bringing it up for discussion.
I guess @huwshimi may correct me if needed, but that's some of the historical context of what we currently have.
Overall react-components started from components extracted initially from MAAS. The initial idea for React components was to "hide" the Vanilla class names and implementation details behind React API of components, so technically as a React developer you wouldn't need to know about Vanilla class names, modifiers, HTML structure etc. You would have a React component with its props and that would be the API you use.
Initial components were based on this idea, so the props were whatever made sense from React component point of view, not exactly from Vanilla point of view.
Over time we noticed that the idea of "not needing to know about Vanilla" was false in practice, we did need to be able to add custom class names here and there, we did need to understand how components can be nested in markup to use them effectively, etc. Even if we "hide" the modifiers under the appearance
prop, you may still want or need to understand that that basically translates to --modifier
in a class name.
At this moment we have quite a solid base of components already, but they are all a bit different, developed by different people in different time, so we are not very consistent in terms of how we pass props to them and how these props are named. We keep react-components in 0.X
version so it kinda doesn't matter, but I think it would be a good time to start thinking about how we want these components and props to look like in foreseeable future. And I guess now that you use these components more extensively in MAAS your feedback on that @petermakowski @huwshimi @Caleb-Ellis would be quite valuable.
When it comes to mapping Vanilla CSS names to React props - I'm not 100% keen on that. Vanilla itself (as much as we try) is not perfect about being consistent, it has a lot of legacy and quirks. It would be a pity if we use them as a base of something that can be better just to be "consistent".
For example the distinction between modifiers p-component--modifier
and flags is-something
is quite arbitrary in Vanilla and definitely not something we should directly map to enums or bool props in React.
But let's start the conversation about it. It would be a nice start to at least consider getting react components to 1.0.0 at some point, and having a cleaner consistent prop API would be a nice part of it definitely.
Thanks for giving a detailed background! I had no idea what was the thinking behind this.
When it comes to mapping Vanilla CSS names to React props - I'm not 100% keen on that. Vanilla itself (as much as we try) is not perfect about being consistent, it has a lot of legacy and quirks. It would be a pity if we use them as a base of something that can be better just to be "consistent".
I do understand your concerns and I'd never push for consistency just for the sake of it.
Consistency in this context does have a relatively big impact on developer's experience though. Inconsistent props/classes naming is precisely what I found confusing about react-components
when I started working with them and it increased the learning curve.
Ideally once you learn a concept in vanilla, you would know how to apply it right away in react-components without having to read the docs yet again. As an example, what happened in my case was - I had to check each docs way more than I'd normally would when dealing with a new design system/component library, often confusing react-components and vanilla props/class names when jumping between projects.
But let's start the conversation about it. It would be a nice start to at least consider getting react components to 1.0.0 at some point, and having a cleaner consistent prop API would be a nice part of it definitely.
Awesome, I'm excited to get this moving and wonder what @Caleb-Ellis and @huwshimi thoughts on this are.
We could start by discussing naming for boolean props. Normally what I'd expect is that the is
/has
/should
prefix convention to be followed for either all or none of boolean props. What we seem to have at the moment though is a set of props that are neither consistent internally (within react-components) nor with vanilla. A few examples below.
The reason why I start here is that it's relatively easy to enforce a naming convention for boolean props with an eslint rule once we decide on a convention. https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/boolean-prop-naming.md#rule
vanilla framework | react-components |
---|---|
has-icon | hasIcon |
is-inline | inline |
is-disabled | disabled |
is-tick-element | isTickElement |
has-overflow | hasOverflow |
p-card--overlay | overlay |
p-table__expanding-panel | expanding |
Yep @bartaz pretty much summarised the whole journey to get to where we are. I would agree that the goal of not needing to know about Vanilla to use the component library was the wrong move - instead of treating them as two separate projects entirely, the reality is that react-components is a layer that sits on top of Vanilla and so is intrinsically linked.
Given the inconsistencies in Vanilla class naming itself, the ideal course of action would probably be to standardise it there first before changing anything in react-components. But I imagine that could take a reeeeally long time to happen. In the meantime I think there is a benefit in aligning what we currently have in Vanilla with the props in react-components beyond just "consistency" - like @petermakowski mentioned I think it would lower the cognitive load of having to translate between Vanilla and react-components. I have the same problem myself of having to figure out how they map together sometimes.
So I reckon we could start following Vanilla's naming even if it's not ideal. Following this pattern would introduce a more brittle react-components API (since the internal class names are no longer abstracted away completely) but I think it's an overall better decision for the team. Plus, if we get started aligning the two and run into pain points, that could be the time in which we decide the ideal naming that works across both projects.
As far as boolean props are concerned, I think state class names ("is-state"
, "has-state"
) should just be camelCased to make the prop name (isState
, hasState
). Class modifiers I'm not so sure about... maybe for now they could also just be camelCased ("p-pattern--some-modifier"
=> someModifier
) but this will definitely need a bit more thought.
Also I'm hesitant to introduce an eslint rule for enforcing boolean prop names, especially if we decide that Vanilla is what defines the names.
@petermakowski I'm moving it to Icebox for now. Do you think there is anything actionable on it at this point?
Triage: That's a discussion that should be revisited when implementing new architecture and new React components.
What
We should have a common way of translating base/modifier classes that exist in vanilla to props in
react-components
.For example, the
help
prop inInput
could be namedformHelpText
instead to matchp-form-help-text
from Vanilla. There are many examples for inconsistencies like that.Why
This would make working with react-components more intuitive for people who are already familiar with the vanilla framework (and vice-versa).
Other considerations
If we agree on a naming convention it might be worth updating all components props at some point even though there will be breaking changes.
Originally raised by @bartaz https://github.com/canonical-web-and-design/react-components/pull/761#discussion_r850305544