Skip to content
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

chore: Add FormFieldGroup component and density example #2865

Merged
merged 80 commits into from
Sep 20, 2024

Conversation

mannycarrera4
Copy link
Contributor

@mannycarrera4 mannycarrera4 commented Aug 9, 2024

Summary

Fixes: #2805, #2806, #2820, #2807

Release Category

Components

Release Note

  • We've added a new FormFieldGroup component to use when grouping inputs like checkboxes and radio inputs that need to be associated to one another. Its API matches the FormField API where you have error prop as well as id isRequired and orienation.

  • orientation has been added back to useFormFieldModel to better support sub component styling.

  • Styles have been cleaned up to use gap for proper spacing between labels, inputs and hint text.

  • Added a new FormField.Field component to ensure proper alignment between label and inputs regardless of orientation and hint text. Ensure to wrap your inputs and hint text in this component.


Checklist

For the Reviewer

  • PR title is short and descriptive
  • PR summary describes the change (Fixes/Resolves linked correctly)
  • PR Release Notes describes additional information useful to call out in a release message or removed if not applicable
  • Breaking Changes provides useful information to upgrade to this code or removed if not applicable

Where Should the Reviewer Start?

Areas for Feedback? (optional)

  • Code
  • Documentation
  • Testing
  • Codemods

Testing Manually

Screenshots or GIFs (if applicable)

Thank You Gif (optional)

Copy link

cypress bot commented Aug 9, 2024

Workday/canvas-kit    Run #7776

Run Properties:  status check passed Passed #7776  •  git commit 4c2dc6dc7b ℹ️: Merge a11e36dd1c7c556b2bc54acc69455290d4b14c6a into 442af058736ce712c0033c941e71...
Project Workday/canvas-kit
Branch Review mc-ff-example
Run status status check passed Passed #7776
Run duration 03m 21s
Commit git commit 4c2dc6dc7b ℹ️: Merge a11e36dd1c7c556b2bc54acc69455290d4b14c6a into 442af058736ce712c0033c941e71...
Committer Manuel Carrera
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 14
Tests that did not run due to a developer annotating a test with .skip  Pending 16
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 854
View all changes introduced in this branch ↗︎
UI Coverage  21.79%
  Untested elements 1768  
  Tested elements 490  
Accessibility  99.21%
  Failed rules  5 critical   5 serious   0 moderate   2 minor
  Failed elements 228  

@mannycarrera4 mannycarrera4 marked this pull request as ready for review August 15, 2024 22:36
@mannycarrera4 mannycarrera4 added the ready for review Code is ready for review label Aug 19, 2024
@mannycarrera4 mannycarrera4 added approved Code has been reviewed and approved (ship it) on hold PR is on hold until further notice 12.x and removed ready for review Code is ready for review labels Sep 19, 2024
cypress.config.ts Outdated Show resolved Hide resolved
@@ -24,7 +24,7 @@ const radioGroupStencil = createStencil({
borderRadius: system.shape.x1,
gap: system.space.x2,
padding: `${px2rem(10)} ${system.space.x3} ${system.space.x2}`,
margin: `${calc.negate(system.space.x1)} ${calc.negate(system.space.x3)}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using margin, we're using gap for spacing

@mannycarrera4
Copy link
Contributor Author

Note: Accepted build: https://www.chromatic.com/build?appId=5d854c26ba934e0020f5e98a&number=9419 on chromatic.

The visual difference in height it due to the removal of margin on the radio group container. When used with formfield, the gap should handle proper spacing. When a radio group is rendered by itself it will show the visual difference.

borderRadius: system.shape.x1,
gap: system.space.x2,
padding: `${px2rem(10)} ${system.space.x3} ${system.space.x2}`,
margin: `0 ${calc.negate(system.space.x3)}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed margin bottom/top, we should accomplish this with gap

@@ -39,7 +39,7 @@ const Container = styled('div')<Pick<RadioGroupProps, 'error' | 'grow' | 'theme'
boxSizing: 'border-box',
borderRadius: borderRadius.m,
padding: `${space.xxxs} ${space.xs}`,
margin: `-${space.xxxs} -${space.xs}`,
margin: `0 -${space.xs}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using gap for spacing

@@ -148,7 +145,7 @@ export const SelectCard = createSubcomponent('div')({
);
});

export interface SelectProps extends Themeable, ExtractProps<typeof Combobox> {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removal of themable

Co-authored-by: Josh <44883293+josh-bagwell@users.noreply.github.com>
@mannycarrera4 mannycarrera4 added automerge and removed on hold PR is on hold until further notice labels Sep 20, 2024
@alanbsmith alanbsmith merged commit 87dbe31 into Workday:prerelease/major Sep 20, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
12.x approved Code has been reviewed and approved (ship it) automerge
Projects
Status: ✅ Done
4 participants