-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat: allow custom help text class #761
Conversation
Demo starting at https://react-components-761.demos.haus |
/** | ||
* Optional class(es) to pass to the help text element. | ||
*/ | ||
helpClassName?: ReactNode; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😆
There was a problem hiding this comment.
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.
There was a problem hiding this 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
Done
QA
Storybook
To see rendered examples of all react-components, run:
QA in your project
from
react-components
run:Install the resulting tarball in your project with:
QA steps
Fixes
Fixes: #760