New Form Layout #982
New Form Layout #982
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new form layout system with multiple reusable components and provides a playground for testing form patterns. The changes include new React components for flexible form layouts, comprehensive SCSS styling, code quality improvements in existing stylesheets, and documentation corrections.
- Adds new FormLayout, FormGroupNew, and FormGroupItemNew components to support flexible grid-based and flex-based form layouts
- Introduces FormPlayground demonstrating two different form patterns (multiple form groups vs single form group)
- Refactors searchbar styles to use CSS variables and improves code formatting consistency
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/pages/playgrounds/react-playgrounds/FormPlayground.tsx | New playground component showcasing form layout patterns with various input combinations |
| examples/pages/playgrounds/react-playgrounds/Index.tsx | Exports the new FormPlayground component |
| examples/index.js | Registers the FormPlayground component in the routing configuration |
| app-typescript/components/Form/FormLayout.tsx | New component providing configurable form layout wrapper with fieldset support |
| app-typescript/components/Form/FormGroupNew.tsx | New component for grouping form items with optional grid layout |
| app-typescript/components/Form/FormGroupItemNew.tsx | New component for individual form items with responsive column spanning |
| app-typescript/components/Form/index.tsx | Exports the new form components |
| app/styles/form-elements/_forms-general.scss | Adds comprehensive styling for new form layout system with responsive grid support |
| app/styles/components/_sd-searchbar.scss | Refactors to use CSS variables and improves formatting consistency |
| app/styles/_reboot.scss | Minor comment formatting improvement |
| examples/pages/components/utilities/TextUtilities.tsx | Fixes CSS variable name in documentation from --text-size-x-small to --text-size-xx-small |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| let options3 = [ | ||
| { | ||
| value: {name: 'Norvegian'}, |
There was a problem hiding this comment.
Typo in comment: "Norvegian" should be "Norwegian"
| value: {name: 'Norvegian'}, | |
| value: {name: 'Norwegian'}, |
| changeStatus(item: any, status: string) { | ||
| if (item.status.includes(status)) { | ||
| item.status.splice(item.status.indexOf(status), 1); | ||
| } else { | ||
| item.status.push(status); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The changeStatus method is defined but never used in the component. Consider removing it if it's not needed, or implementing its functionality if it was intended to be used.
| changeStatus(item: any, status: string) { | |
| if (item.status.includes(status)) { | |
| item.status.splice(item.status.indexOf(status), 1); | |
| } else { | |
| item.status.push(status); | |
| } | |
| } |
| children: React.ReactNode; | ||
| spaces?: 'compact' | 'standard' | 'relaxed'; // defaults to 'standard' | ||
| marginBottom?: '0' | '1' | '2' | '3' | '4'; // multipliers of 8px (base increment); defaults to '2' (16px) | ||
| legend?: string; // provides anoptional legend for the form layout/fieldset (can be used to group items together) |
There was a problem hiding this comment.
Typo in comment: "anoptional" should be "an optional"
| legend?: string; // provides anoptional legend for the form layout/fieldset (can be used to group items together) | |
| legend?: string; // provides an optional legend for the form layout/fieldset (can be used to group items together) |
| .form__group-new--has_row-label { | ||
| .form__group-new__wrapper { | ||
| display: flex; | ||
| flex-direction: column; | ||
| width: 100%; | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate CSS rule block. Lines 397-403 and 405-411 define the exact same .form__group-new--has_row-label .form__group-new__wrapper rules. One of these blocks should be removed.
| .form__group-new--has_row-label { | |
| .form__group-new__wrapper { | |
| display: flex; | |
| flex-direction: column; | |
| width: 100%; | |
| } | |
| } |
| .form-group-new-item--auto-width { | ||
| flex-grow: 0; | ||
| min-width: 0; | ||
| flex-basis: auto; | ||
| } |
There was a problem hiding this comment.
Duplicate CSS rule. Lines 542-546 define .form-group-new-item--auto-width with the same properties already defined within .form-group-new-item at lines 516-520. This duplication should be removed.
| .form-group-new-item--auto-width { | |
| flex-grow: 0; | |
| min-width: 0; | |
| flex-basis: auto; | |
| } |
| --form-group-col-gap: var(--gap-3); | ||
| } | ||
| } | ||
| @container form-container (width<= 500px) { |
There was a problem hiding this comment.
Missing space after <= operator. Should be (width <= 500px) for consistency with line 547.
| @container form-container (width<= 500px) { | |
| @container form-container (width <= 500px) { |
|
@fritzSF I have a bunch of technical comments on code style, etc. Do you want me to just make a commit to address them? |
@thecalcc Yes, please go ahead! The Copilot-generated review comments aren't particularly helpful at this stage. I see this more as a proof of concept, the CSS needs cleanup and optimization, the component needs renaming, and we'll need proper documentation. |
|
It looks good to me. As Konstantin mentioned, it needs some polishing but I understand it is a work in progress. What would be the next step @fritzSF? is there anything you would need from our side? |
3 participants
FormLayout,FormGroupNew,FormGroupItemNewNote: Component names are temporary, only for testing