-
Notifications
You must be signed in to change notification settings - Fork 74
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
PatternFly React: Our own FormFieldset and FormLegend components #1339
Conversation
ac197ae
to
66e75ba
Compare
Codecov Report
@@ Coverage Diff @@
## master #1339 +/- ##
==========================================
- Coverage 93.17% 90.76% -2.41%
==========================================
Files 2518 2216 -302
Lines 83583 68940 -14643
==========================================
- Hits 77877 62574 -15303
- Misses 5706 6366 +660
Continue to review full report at Codecov.
|
66e75ba
to
e112c20
Compare
e112c20
to
e910308
Compare
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.
Approving, just leave some comments but totally optional, would like to see it in action
className={css(styles.formFieldset, isInline ? getModifier(styles, 'inline', className) : className)} | ||
> | ||
{label && ( | ||
<label className={css(styles.formLabel)} htmlFor={fieldId}> |
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.
There is a PF Label component, we could use that to not rewrite already existing components.
But I think it will be lighter and simpler using only a vanilla <label>
tag here, considering this is a temporary solution until core PF team develops the <Fieldset />
component. So it's OK for me
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.
Funny thing is they use the exactly samelabel
composition for their FormGroup
component and not the Label
component.
@@ -0,0 +1,68 @@ | |||
// @flow | |||
// TODO: Replace this component when patternfly-react implements it. |
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.
Do you know if is there an open issue or PR created for this? May be useful to have it, to track progress
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.
Nope, not a single thing. Also, I didn't open one because they don't have it in their library pattern for v4 nor v3 (following they contribution guidelines). I've asked about this in their slack, but no answer.
What this PR does / why we need it:
the patternfly-react lib hasn't implemented still the components for fieldset and legend, so these ones are the ones we will be using until they release their own.
Needed for #1315
Which issue(s) this PR fixes
Verification steps
Special notes for your reviewer:
These components will be removed when patternfly team release the official ones.