-
Notifications
You must be signed in to change notification settings - Fork 4
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: add FormGroup component #809
Conversation
Codecov Report
@@ Coverage Diff @@
## main #809 +/- ##
==========================================
+ Coverage 95.58% 95.68% +0.09%
==========================================
Files 136 139 +3
Lines 975 997 +22
Branches 134 137 +3
==========================================
+ Hits 932 954 +22
Misses 43 43
Continue to review full report at Codecov.
|
@@ -0,0 +1,3 @@ | |||
// @flow |
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.
Should this be removed?
// @flow |
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.
Yes!! Thank you ❤️
<Component className={clsx(className, styles.title)}> | ||
<Text size="sm" weight="bold"> | ||
{children} | ||
</Text> | ||
</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.
Does this result in HTML like with an extra P tag:
<legend>
<p class="stuff">Some text</p>
</legend>
?
If so, I wonder if it's worth thinking about reducing unnecessary elements. Maybe with something like
<Text as={Component} className={clsx(className, styles.title)} size="sm" weight="bold">
{children}
</Text>
Although that may not really be worth it. And would probably require updating the Text component to accept more things for as
.
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.
And would probably require updating the Text component to accept more things for as.
Looks like it. Maybe we should make that change anyway and then update this.
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.
Yeah; I stacked them so it was easier to make an alpha version with both of them on it. I'll merge them into |
b2610e2
to
b3856ac
Compare
49ea774
to
ab0bfbf
Compare
8f7fa60
to
2278b94
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.
size-limit report
Path | Size |
---|---|
components | 68.48 KB (+1.37% 🔺) |
styles | 4.44 KB (0%) |
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
No idea why a size increase of 1.37% would cause the size limit action to block merging. Maybe it's enforcing a total size limit of the package or something. Couple options:
|
Cool, I'll just dismiss it for now. Thanks for verifying! 👍 |
We're cool with the package getting a little bigger as we add more components. The size does seem a little high for this component (?), but it's within reason.
Summary:
Clubhouse ticket: https://app.shortcut.com/czi-edu/story/169595/button-group-form-group-move-to-eds
Location in
traject
: https://github.com/FB-PLP/traject/tree/master/app/assets/javascripts/v2/core/EDSCandidates/FormGroupScreenshots
Test Plan: