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

feat: allow custom help text class #761

Merged

Conversation

petermakowski
Copy link
Contributor

Done

  • allow custom help text class

QA

Storybook

To see rendered examples of all react-components, run:

yarn start

QA in your project

from react-components run:

yarn build
npm pack

Install the resulting tarball in your project with:

yarn add <path-to-tarball>

QA steps

  • Steps for QA.

Fixes

Fixes: #760

@webteam-app
Copy link

Demo starting at https://react-components-761.demos.haus

/**
* Optional class(es) to pass to the help text element.
*/
helpClassName?: ReactNode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess would be good to have some convention for adding those child class names, I think some other components do it as well (don't remember which one).

Not sure how detailed or strict we want to be on which elements have configurable class name and which don't. But at least some prop naming convention would be good.

elementClassName seems fine for start (and consistency with default className.

Copy link
Contributor Author

@petermakowski petermakowski Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this is needed (and I like your suggested naming convention).

I think we should have a common way of translating base/modifier classes that exist in vanilla to props in react-components as well, e.g. the help prop in Input should probably be named formHelpText instead to match p-form-help-text from Vanilla. There are many examples for inconsistencies like that.

This would make working with react-components more intuitive for people who are already familiar with the vanilla framework (and vice-versa).

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue for this #762

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that the related element in vanilla has a base class p-form-help-text, would you consider a name of the element to be formHelpText? That would result in formHelpTextClassName instead of helpClassName.

A bit lengthy, but I don't think that matters - having prop names that are long but consistent is much better than having props that are shorter but different in vanilla and react-components.

Copy link
Member

@bartaz bartaz Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately form components in Vanilla have very inconsistent and lengthy names. There is no "p-form", but we do have "p-form-others"… and there are more issues like that, so forms are definitely not something we should base the naming convention on 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, should we go with my initial suggestion (helpClassName) then? It's consistent with the related help prop that already exists on this component.

Copy link
Contributor

@Caleb-Ellis Caleb-Ellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think it's good that its consistent with the current help prop and we can define a better naming convention later on

@petermakowski petermakowski merged commit 15f5a5b into canonical:main Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature 🎁 New feature or request Review: Code +1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input: Allow customising help text className
4 participants