docs(textfield,textarea): migrating docs to storybook #3204
Conversation
|
File metricsSummaryTotal size: 4.30 MB* 🎉 No changes detected in any packages * Size determined by adding together the size of the main file for all packages in the library.* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
castastrophe
force-pushed
the
aramos-adobe/css942-textfield-migration-docs
branch
from
020c296 to
cbdaf93
Compare
|
🚀 Deployed on https://pr-3204--spectrum-css.netlify.app |
castastrophe
added
the
skip_vrt
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice work on this. 👍 This addressed my previous feedback. My code comments are for some small things that aren't really blockers.
For CSS-729, about increasing Chromatic coverage, I did see two things that are important to add to the .test.js files for both components:
- Help text: since help text is nested within the text field component, and has unique styles in the CSS, we should cover this in VRTs. This also should make sure to include help text with side label, which has some grid alignment styles.
- Quiet: I saw the Jira issue said "can probably skip the 'quiet' variants for now, since those are going away in s2", but things have changed since that was written. We need to make sure these S1 things such as quiet are covered, especially with the foundations work where we're checking for any regressions.
There was a problem hiding this comment.
This is coming along nicely! This is my first pass, and I mainly focused on comparing text field/text area in this branch to the S2 branch.
I did notice that we could probably trim down the repetition for the new textarea files, especially if we structure it like meter, which is why I am requesting changes right now. I'll go through tomorrow morning and do a better review on the docs page content 😄
Also- I do apologize for some of the comments that seem similar. There's a lot of the same stuff going on, and now text field and text area are separate and I didn't want to forget to put a comment in one of those files!
There was a problem hiding this comment.
I focused more on the content of the docs page, and I know I have a lot. If you want to pair on any of it, let me know!
-
I left a couple comments about using the
Templateinstead ofTextFieldGroup. Personally, since most of these stories (as they are currently) are only using a single text field component anyways, let's just use theTemplate. -
There's quite a few states missing from the docs page: focused, keyboard-focused were the main culprits. But then there's all of the invalid-disabled, invalid-focus, quiet-disabled, etc. I left a comment about maybe using something like
Stepperas a model to showcase all of those states in a single story. -
Does any of the migration guide need to be moved into the
CHANGELOG? That migration guide is pretty extensive, so we should be sure we've captured all of it somewhere. If you already combed through that- fabulous! -
I vote to structure this textfield directory more like the fieldlabel (that houses form) and progressbar (that houses meter). I think we could separate out any of the textarea markup into a
textarea.template.jsfile. That would just keep things nice and organized.
There was a problem hiding this comment.
You are amazing for going through all of the feedback! This is looking so stinkin' good!
I think the biggest things that are causing me to pause before approving is the character count in both test files, and then maybe showcasing the default focus and keyboard focus on the docs page (we have those states in the test preview, and they're in the controls, but they aren't on the docs page).
WAY TO GO! 👏👏👏👏👏👏
There was a problem hiding this comment.
I love all the changes! This looks fantastic and I'm so happy we got the textarea all split off now. Thanks for taking this on- it was a beast!
A couple of non-blocking things I noticed:
- In a couple of the text area stories, it looks like the
labelText: "Comments"was redefined. It doesn't really need to be, sincelabelText: "Comments"is the already default arg. 👍 - I love the updated
consts you made to handle all of the focused/keyboard focused states between default & quiet! Nice call 👍 - Should we talk about a deprecation strategy for that
multilinearg at a team meeting? With these separated, maybe once s2 is officially released, that would be a good time to make which component to use a little clearer (by removingmultiline)? I like that you kept the arg and thewhen(multiline)for now, but what does the future hold for that arg with the components now separated? I'm not sure how we handle it. - We talked about the wonky quiet + help text + keyboard focus. We'll make a follow up card (just looking quickly, it's probably a CSS bug). We should apparently add quiet + help text to the tests now! 😆
| @@ -8,31 +8,31 @@ export const TextAreaGroup = Variants({ | |||
| { | |||
| testHeading: "Text area with value", | |||
| displayLabel: true, | |||
| multiline: true, | |||
castastrophe
added
run_vrt
|
@aramos-adobe Can you rebase your branch and take a look at the VRT changes to see that these are the expected updates: https://www.chromatic.com/build?appId=64762974a45b8bc5ca1705a2&number=3505 |
Done ✅ |
castastrophe
removed
skip_vrt
castastrophe
added
run_vrt
castastrophe
force-pushed
the
aramos-adobe/css942-textfield-migration-docs
branch
from
f388a06 to
eba71c1
Compare
castastrophe
dismissed
jawinn’s stale review
Out of office and review feedback has been addressed
Migrating the textfield and textarea documentation to the Storybook.
Migrating the textfield and textarea documentation to the Storybook.
Migrating the textfield and textarea documentation to the Storybook.
Migrating the textfield and textarea documentation to the Storybook.
Migrating the textfield and textarea documentation to the Storybook. Co-authored-by: aramos-adobe <[email protected]>
Description
Migrating docs for Textfield and Textarea.
Validation steps
Textarea/Textfield docs:
textfieldstories folderinValidstate example is includedRegression testing
Validate:
Screenshots
To-do list