feat(rangeinput): creates form RangeInput component, test, and stories #194

duncan-truss · 2020-05-21T22:11:00Z

Description

This PR adds the USWDS Range component to the available form inputs.

The component renders as an input of type range and an optional Label.

The min, max, step, and value fields are optional and default to:
min: 0
max: 100
step: 1
value: max/2

I copied the ref code from somewhere else but don't know entirely why it's necessary or all of the possible types it could be. I think I remember there is an old deprecated way to do refs from the official tutorial.

Open Questions

Should the label be excluded all together from the component or passed whole instead of built out of label and hint attributes?
The USWDS guidance recommends that you label the min and max ends, should we offer that functionality?
Should the component accept any event listeners?
I added the list prop to support linking to a datalist element. This has next to no browser visual support (ticks in chrome if the usa-range class isn't applied) at the moment.
Similar to the start and end labels should there be an <output> field that displays the slider value or would that be handled externally by something that uses an event listener?
Firefox accepts an orient="vertical" attribute but everyone else just uses CSS so I omitted it

MDN input range element documentation

#81

eamahanna

Looks good :)

eamahanna · 2020-05-22T00:12:20Z

Here are my thoughts on the questions you listed in your description:

Should the label be excluded all together from the component or passed whole instead of built out of label and hint attributes?

I think it is fine including the label or not. The text input component does not include a label however.

The USWDS guidance recommends that you label the min and max ends, should we offer that functionality?

I think there should at least be a story showing this. I am unsure if the component should contain them because I don't know how they are suppose to be styled. Maybe there is guidance on that.

Should the component accept any event listeners?

I don't think you need to include a listener explicitly. There is on onChange prop defined in the React.InputHTMLElement interface

 interface InputHTMLAttributes<T> extends HTMLAttributes<T> {
        accept?: string;
        alt?: string;
        autoComplete?: string;
        autoFocus?: boolean;
        capture?: boolean | string; // https://www.w3.org/TR/html-media-capture/#the-capture-attribute
        checked?: boolean;
        crossOrigin?: string;
        disabled?: boolean;
        form?: string;
        formAction?: string;
        formEncType?: string;
        formMethod?: string;
        formNoValidate?: boolean;
        formTarget?: string;
        height?: number | string;
        list?: string;
        max?: number | string;
        maxLength?: number;
        min?: number | string;
        minLength?: number;
        multiple?: boolean;
        name?: string;
        pattern?: string;
        placeholder?: string;
        readOnly?: boolean;
        required?: boolean;
        size?: number;
        src?: string;
        step?: number | string;
        type?: string;
        value?: string | string[] | number;
        width?: number | string;

        onChange?: ChangeEventHandler<T>;   }

I added the list prop to support linking to a datalist element. This has next to no browser visual support (ticks in chrome if the usa-range class isn't applied) at the moment.

Given that it is optional and defined in the InputHTMLAttributes, I don't think it needs to be an explicit prop, but you can show it in a story maybe?

Similar to the start and end labels should there be an field or would that be handled externally by something that uses an event listener?

I think this is the inputHTMLAttribute onChange prop

duncan-truss · 2020-05-22T14:11:22Z

Here are my thoughts on the questions you listed in your description:

Should the label be excluded all together from the component or passed whole instead of built out of label and hint attributes?

* I think it is fine including the label or not. The text input component does not include a label however.

The USWDS guidance recommends that you label the min and max ends, should we offer that functionality?

* I think there should at least be a story showing this. I am unsure if the component should contain them because I don't know how they are suppose to be styled. Maybe there is guidance on that.

Should the component accept any event listeners?

* I don't think you need to include a listener explicitly. There is on onChange prop defined in the React.InputHTMLElement interface

 interface InputHTMLAttributes<T> extends HTMLAttributes<T> {
        accept?: string;
        alt?: string;
        autoComplete?: string;
        autoFocus?: boolean;
        capture?: boolean | string; // https://www.w3.org/TR/html-media-capture/#the-capture-attribute
        checked?: boolean;
        crossOrigin?: string;
        disabled?: boolean;
        form?: string;
        formAction?: string;
        formEncType?: string;
        formMethod?: string;
        formNoValidate?: boolean;
        formTarget?: string;
        height?: number | string;
        list?: string;
        max?: number | string;
        maxLength?: number;
        min?: number | string;
        minLength?: number;
        multiple?: boolean;
        name?: string;
        pattern?: string;
        placeholder?: string;
        readOnly?: boolean;
        required?: boolean;
        size?: number;
        src?: string;
        step?: number | string;
        type?: string;
        value?: string | string[] | number;
        width?: number | string;

        onChange?: ChangeEventHandler<T>;   }

I added the list prop to support linking to a datalist element. This has next to no browser visual support (ticks in chrome if the usa-range class isn't applied) at the moment.

* Given that it is optional and defined in the InputHTMLAttributes, I don't think it needs to be an explicit prop, but you can show it in a story maybe?

Similar to the start and end labels should there be an field or would that be handled externally by something that uses an event listener?

* I think this is the inputHTMLAttribute onChange prop

Thank you for these answers!

Since the InputHTMLAttributes contain min, max, and step should I remove them from the RangeInputProps interface along with list?

haworku · 2020-05-22T16:13:20Z

@duncan-truss to answer your questions about the type interface: yes - remove them from there. We don't need to retype them.

suzubara

looking good! My only blocking comment is about removing the label.

suzubara · 2020-05-22T16:55:23Z

src/components/forms/RangeInput/RangeInput.tsx

+  // Range defaults to min = 0, max = 100, step = 1, and value = (max/2) if not specified.
+  const {
+    id = 'range-slider',
+    name = 'range',


this is my 🔥 opinion and is more of a general comment about form inputs, but IMO id and name are so fundamental to building HTML forms that I don't think we need to provide default values, and I also think they should be required in the prop type.

That's great context, keep the opinions coming I don't have any of my own yet.

suzubara · 2020-05-22T16:55:56Z

src/components/forms/RangeInput/RangeInput.tsx

+
+  return (
+    <>
+      {label && (


To answer your question, the label should not be included in the component (following the examples of the other input components, which only render an <input> element). It should be up to the user to add labels.

suzubara · 2020-05-22T16:57:24Z

src/components/forms/RangeInput/RangeInput.tsx

+        max={max}
+        step={step}
+        list={list}
+        {...inputProps}


suggestion: any of these props that aren't handled specifically in our code (ie, className) can be passed in as part of ...inputProps instead of having to be destructured & passed in explicitly.

suzubara · 2020-05-22T18:04:14Z

I copied the ref code from somewhere else but don't know entirely why it's necessary or all of the possible types it could be. I think I remember there is an old deprecated way to do refs from the official tutorial.

This inputRef prop is one that each input should accept, and allows an implementation to have direct access to the input DOM element which is often required for things like form library integration, or managing focus on form elements.

The USWDS guidance recommends that you label the min and max ends, should we offer that functionality?

Since USWDS doesn't provide any examples or styles I think we can leave it for now

haworku · 2020-05-22T18:42:13Z

This inputRef prop is one that each input should accept, and allows an implementation to have direct access to the input DOM element which is often required for things like form library integration, or managing focus on form elements.

Ah, makes sense @suzubara that's great context - I wasn't sure either about this tbh (the use case specifically). This is an example of something we can include in #73 .

Regarding the types, just generally I wonder if something like inputRef should be a shared type in the /forms folder. Maybe we just define it in TextInput for now and export it for use?

trussworks-infra-zz · 2020-05-22T20:16:03Z

	Warnings
⚠️	Changes were made to package.json, but not to yarn.lock - Perhaps you need to run `yarn install`?

Generated by 🚫 dangerJS against 56bb01e

suzubara

there's just some minor cleanup but nothing blocking. looks good!

suzubara · 2020-05-26T16:04:02Z

src/components/forms/RangeInput/RangeInput.stories.tsx

+  </>
+)
+
+const labelHint = <>(Some hint)</>


it looks like there are a few unused variables now that can be deleted. FWIW I think it would be fine to include a story that renders the RangeInput alongside a Label if you wanted to demo that.

suzubara · 2020-05-26T16:05:17Z

src/components/forms/RangeInput/RangeInput.tsx

+import React from 'react'
+import classnames from 'classnames'
+
+import { Label } from '../Label/Label'


this import can be deleted

* Bump standard-version from 7.1.0 to 8.0.0 Bumps [standard-version](https://github.com/conventional-changelog/standard-version) from 7.1.0 to 8.0.0. - [Release notes](https://github.com/conventional-changelog/standard-version/releases) - [Changelog](https://github.com/conventional-changelog/standard-version/blob/master/CHANGELOG.md) - [Commits](conventional-changelog/standard-version@v7.1.0...v8.0.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * Created issue templates Created issue templates for feature request & bug report, with default labels. * fix: removes the usa-search class name from the form component (#184) fix #163 * fix: accept id and name props for Search component input field (#183) * fix: accept id and name props for Search component input field * fix: qualify id and name props for input field to not clash with form fix #162 * feat: add dateInput component for forms (#144) * Added dateInput component and dateInputGroup component for forms * fix: extends header menu to accept list props for corresponding id (#188) fix #165 * fix: update gov banner to uswds version 2.7.0 * chore: upgrade uswds to 2.7.0 * docs: add documentation for Trussels (#190) * docs: add documentation for Trussels * docs: clean up readme to meet project standards * docs: add to active maintainers * docs: add first pass of security policy Co-authored-by: HANA <hana@truss.works> * feat: add Footer component #142 (#146) - adds slim, medium, and big footers with mobile styles - adds new components, specifically Address, Footer, FooterNav, FooterExtendedNavList, Logo, SocialLinks * build(deps-dev): bump @testing-library/jest-dom from 5.7.0 to 5.8.0 (#197) * build(deps-dev): bump typescript from 3.9.2 to 3.9.3 (#196) * build(deps-dev): bump @storybook/react from 5.3.18 to 5.3.19 (#198) * build(deps-dev): bump stylelint from 13.4.1 to 13.5.0 (#199) * build(deps-dev): bump lint-staged from 10.2.4 to 10.2.6 (#200) * build(deps-dev): bump @storybook/addon-info from 5.3.18 to 5.3.19 (#201) * feat(rangeinput): creates form RangeInput component, test, and stories (#194) * feat(rangeinput): creates form RangeInput component, test, and stories * feat: add RangeInput component to index.ts for entrypoint export * feat: remove overalpping interface props with HTMLInputAttributes type * feat: require id and name props, remove label, remove redundant unpack * feat: remove unused label import in component, show label usage in story fixes #81 * docs: add landing page and documentation page template stories (#195) * docs: add landing page and documentation page template stories * docs(storybook): add current class to header navigation * ci: lint PR titles instead of commits #159 (#210) * chore: add semantic pr workflow * chore: update workflow * Remove commitlint hook * Clarify PR linting requirement Co-authored-by: HANA <hana@truss.works> * chore(release): 1.4.0 * Remove duplicate entry from changelog Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: Duncan <52669884+duncan-truss@users.noreply.github.com> Co-authored-by: Emily Mahanna <56279459+eamahanna@users.noreply.github.com> Co-authored-by: HANA <hana@truss.works>

feat(rangeinput): creates form RangeInput component, test, and stories

1a37d1a

#81

duncan-truss requested review from suzubara, haworku, sojeri and eamahanna May 21, 2020 22:11

duncan-truss requested review from gidjin, sarboc and tinyels as code owners May 21, 2020 22:11

duncan-truss self-assigned this May 21, 2020

duncan-truss linked an issue May 21, 2020 that may be closed by this pull request

USWDS Component: Range slider #81

Closed

eamahanna approved these changes May 21, 2020

View reviewed changes

feat: add RangeInput component to index.ts for entrypoint export

6fb0d72

feat: remove overalpping interface props with HTMLInputAttributes type

08b1733

suzubara suggested changes May 22, 2020

View reviewed changes

feat: require id and name props, remove label, remove redundant unpack

a6e5fb5

duncan-truss requested a review from suzubara May 22, 2020 20:23

haworku approved these changes May 26, 2020

View reviewed changes

suzubara approved these changes May 26, 2020

View reviewed changes

feat: remove unused label import in component, show label usage in story

56bb01e

duncan-truss merged commit 8e0179f into develop May 26, 2020

duncan-truss deleted the ds-range-slider-component-81 branch May 26, 2020 16:33

suzubara mentioned this pull request May 28, 2020

chore(release): Release 1.4.0 #211

Merged

haworku mentioned this pull request Jun 18, 2020

[WIP] feat(Button): add "as" prop to allow custom tags #263

Closed

haworku mentioned this pull request Jun 29, 2020

chore(release): 1.6.0 #286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rangeinput): creates form RangeInput component, test, and stories #194

feat(rangeinput): creates form RangeInput component, test, and stories #194

duncan-truss commented May 21, 2020 •

edited

Loading

eamahanna left a comment

eamahanna commented May 22, 2020

duncan-truss commented May 22, 2020

haworku commented May 22, 2020

suzubara left a comment

suzubara May 22, 2020

duncan-truss May 22, 2020

suzubara May 22, 2020

suzubara May 22, 2020

suzubara commented May 22, 2020

haworku commented May 22, 2020

trussworks-infra-zz commented May 22, 2020 •

edited

Loading

suzubara left a comment

suzubara May 26, 2020

suzubara May 26, 2020

feat(rangeinput): creates form RangeInput component, test, and stories #194

feat(rangeinput): creates form RangeInput component, test, and stories #194

Conversation

duncan-truss commented May 21, 2020 • edited Loading

Description

Open Questions

eamahanna left a comment

Choose a reason for hiding this comment

eamahanna commented May 22, 2020

duncan-truss commented May 22, 2020

haworku commented May 22, 2020

suzubara left a comment

Choose a reason for hiding this comment

suzubara May 22, 2020

Choose a reason for hiding this comment

duncan-truss May 22, 2020

Choose a reason for hiding this comment

suzubara May 22, 2020

Choose a reason for hiding this comment

suzubara May 22, 2020

Choose a reason for hiding this comment

suzubara commented May 22, 2020

haworku commented May 22, 2020

trussworks-infra-zz commented May 22, 2020 • edited Loading

suzubara left a comment

Choose a reason for hiding this comment

suzubara May 26, 2020

Choose a reason for hiding this comment

suzubara May 26, 2020

Choose a reason for hiding this comment

duncan-truss commented May 21, 2020 •

edited

Loading

trussworks-infra-zz commented May 22, 2020 •

edited

Loading