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

feat(ds-core-form): boilerplate pt 4 #167

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

advl
Copy link
Contributor

@advl advl commented Mar 20, 2025

Done

  • Adds Range Input
  • Adds native Select Input
  • Adds SimpleChoices input (fieldset of radios/comboboxes)
  • Adds useOptionAriaProperties hook in an initial stage
  • Fixes useFieldAriaProperties call of useId
  • Enhances props of components with a union to native HTML properties (e.g. export type TextareaProps = BaseInputProps & React.HTMLProps<HTMLTextAreaElement>; )
  • drive_by: Added a note about the autodocs on the storybook package

QA

  • ! Atm, there are a bunch of type errors. Not ready for final Q/A yet.

PR readiness check

  • PR should have one of the following labels:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • PR title follows the Conventional Commits format.
  • All packages define the required scripts in package.json:
    • All packages: check, check:fix, and test.
    • Packages with a build step: build.

@advl advl requested a review from jmuzina March 20, 2025 17:15
@advl
Copy link
Contributor Author

advl commented Mar 20, 2025

Points of interest for an initial review :

  • Type enhancements for component props (native html props)
  • Usage of Fieldset for the SimpleChoices component
  • Usage of a javascript-read value in the output tag of the Range input
  • the useOptionAriaProperties hooks

{...register(name, registerProps)}
/>
<output htmlFor={id} className="ds range-output">
{value}
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that the output is currently empty until the user changes the slider value (no initial value is loaded).

Even when I modified the range story to use a pre-filled value, the output was not populated until after I interacted with the form.

Copy link
Contributor Author

@advl advl Mar 20, 2025

Choose a reason for hiding this comment

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

fixed
a dictionary of defaultValues needs to be provided to the Form at instantiation - see the story on the latest commit for an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a dictionary of defaultValues needs to be provided to the Form at instantiation - see the story on the latest commit for an example

Copy link
Member

@jmuzina jmuzina Mar 20, 2025

Choose a reason for hiding this comment

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

Ah, I was consuming the form in an unintended way I think:

<Field type="range" value={3}/>

value is valid here as it comes from the InputElement props. You may want to disallow that prop if we don't intend for the field to be used in this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants