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

Define prop naming conventions #762

Open
petermakowski opened this issue Apr 14, 2022 · 5 comments
Open

Define prop naming conventions #762

petermakowski opened this issue Apr 14, 2022 · 5 comments
Labels
P3 Triaged Issue has been reviewed as part of legacy backlog grooming (project P3). Question ❓ Further information is requested Triaged: new architecture Triaged, to be addressed in new architecture of the design system

Comments

@petermakowski
Copy link
Contributor

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 in Input could be named formHelpText instead to match p-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 #761 (comment)

@bartaz
Copy link
Member

bartaz commented Apr 14, 2022

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.

@petermakowski
Copy link
Contributor Author

petermakowski commented Apr 15, 2022

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

Boolean props in various react-components and their vanilla framework equivalents

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

@Caleb-Ellis
Copy link
Contributor

Caleb-Ellis commented Apr 20, 2022

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.

@cristinadresch cristinadresch added the Question ❓ Further information is requested label Apr 25, 2022
@bartaz
Copy link
Member

bartaz commented Jun 22, 2022

@petermakowski I'm moving it to Icebox for now. Do you think there is anything actionable on it at this point?

@bartaz
Copy link
Member

bartaz commented Sep 30, 2024

Triage: That's a discussion that should be revisited when implementing new architecture and new React components.

@bartaz bartaz added P3 Triaged Issue has been reviewed as part of legacy backlog grooming (project P3). Triaged: new architecture Triaged, to be addressed in new architecture of the design system labels Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Triaged Issue has been reviewed as part of legacy backlog grooming (project P3). Question ❓ Further information is requested Triaged: new architecture Triaged, to be addressed in new architecture of the design system
Projects
None yet
Development

No branches or pull requests

4 participants