fix: blank field group docs and read-only radio alignment #3164
Conversation
🦋 Changeset detectedLatest commit: 3054d94 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Fixes the Docs page for Field group displaying blank, because the template lost its default arg value for items.
Correct an issue with the alignment of a readonly radio's label element within field group and flex layouts. The use of `auto` on the margin was previously pushing the label element all the way to the opposite side when within a field group.
jawinn
force-pushed
the
jawinn/fix-field-group-docs-and-readonly-radio
branch
from
ed577d2
to
3054d94
Compare
|
🚀 Deployed on https://pr-3164--spectrum-css.netlify.app |
File metricsSummaryTotal size: 4.31 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsradio
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
| (passthroughs, context) => Radio({ | ||
| ...passthroughs, | ||
| id: "apple", | ||
| label: "Apples are best", | ||
| customClasses: ["spectrum-FieldGroup-item"], | ||
| }, context), |
There was a problem hiding this comment.
We'll have to edit some of the items for certain stories. Right now, all of the checkbox variants are showing radio buttons instead of checkboxes.
There was a problem hiding this comment.
Also, does the toggle for inputType need to do anything else but change the role from radiogroup to group? It's doing that right now, all is good- I just expected it to change the component shown, but that seems like work for another PR.
There was a problem hiding this comment.
Re: only getting radios for control group now--this means our control for radios vs. checkboxes is also out of order (and has been for awhile) 😕 Our template doesn't really support switching based on inputType the way it did before, so I wonder if it'd be necessary to create separate stories for a checkbox group and radio group? That could easily be a separate effort outside of this PR if it doesn't seem doable here though.
There was a problem hiding this comment.
Now that you've brought the Docs back on FieldGroup, there are lots of other things going on here with the controls particularly that have my attention. Here's what I've noticed (I already mentioned some of these below):
- We have a stray
labelcontrol in the controls - We lost the Help text, which maybe is ok (although we are testing it here in Chromatic, so it seems like we ought to keep it), but since that was the only thing that visibly changed when the
invalidcontrol is turned on, now theinvalidcontrol doesn't appear to do anything. - We gained a
requiredcontrol at some point after the docs to storybook for this was completed and that doesn't seem to do anything, I think maybe we just need to pass that arg down to the FieldLabel in the template. - Our controls for checkbox/radio don't do anything, we can only see radios
- The testing preview does show checkboxes and radios but shows checkboxes only with horizontal positioning (and doesn't show radios), do we think this is sufficient to test them? It feels to me like we could be missing something by not testing radios and checkboxes in all positions, but I could be convinced otherwise.
Addressing these separately would definitely be ok as far as I'm concerned since these aren't things that broke in this PR and they weren't in the original scope of the work, so I don't know that all of this needs to be fixed before we merge, but definitely all things we should address with fieldgroup at some point!
Also I'm going to mention the docs to storybook PR for fieldgroup here and link the preview for easy reference in case we want to go back and see how it looked before when it worked: #2827
| @@ -62,6 +62,27 @@ export default { | |||
| inputType: "radio", | |||
| labelPosition: "top", | |||
| layout: "vertical", | |||
| label: "Select one of the following options:", | |||
There was a problem hiding this comment.
We have a { table: { disable: true } } on fieldLabel in L55 here but I think somewhere along the way the arg changed from fieldLabel to label and now we have a stray label control showing in Storybook that we would probably want to hide:
| (passthroughs, context) => Radio({ | ||
| ...passthroughs, | ||
| id: "apple", | ||
| label: "Apples are best", | ||
| customClasses: ["spectrum-FieldGroup-item"], | ||
| }, context), |
There was a problem hiding this comment.
Re: only getting radios for control group now--this means our control for radios vs. checkboxes is also out of order (and has been for awhile) 😕 Our template doesn't really support switching based on inputType the way it did before, so I wonder if it'd be necessary to create separate stories for a checkbox group and radio group? That could easily be a separate effort outside of this PR if it doesn't seem doable here though.
Description
Includes two small fixes:
itemsin its Template, causing nothing to render within the field group markup.margin-inline-startbeing set toautoinstead of0for read-only. In a flex container,autoon the margin of a flex item will automatically extend its specified margin to occupy the extra space.How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
Regression testing
Validate:
Screenshots
Read-only bug. Previous behavior:
Fixed behavior:
To-do list