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

Extract InputRoot component #827

Merged
merged 2 commits into from
Feb 8, 2023
Merged

Extract InputRoot component #827

merged 2 commits into from
Feb 8, 2023

Conversation

lyzadanger
Copy link
Contributor

This PR establishes a root component for textual input elements. The purpose of this is to be able to share design patterns and behavior across those input elements. I'm terming this a "Root" element, as distinct from a "Base" element, because it is private to the package. Early days here; I want to feel this out.

Expect to see a Select component based on this imminently. A TextArea component is probably not far off, either, but doesn't have as immediate of a need as Select.

There should be no visible or behavioral changes to the existing Input component. You can verify that on the Input pattern-library page.

{...htmlAttributes}
type={type}
// @ts-ignore-next
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ref-typing purgatory...I should add more comment here but I don't really know what to say.

return (
<input
<Element
data-component="InputRoot"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In all normal uses of this component this data-component attribute should be overridden. That's by design.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own understanding (not related with this PR per se). What's the purpose of data-component when used by consumer code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly as a debugging affordance. It's easier to see when inspecting the DOM where elements come from.

@@ -10,17 +10,4 @@ const contentFn = (Component, props = {}) => {

describe('Input', () => {
testPresentationalComponent(Input, { createContent: contentFn });

it('should warn in console if accessibility attributes are missing', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now the responsibility of InputRoot

Create a new `InputRoot` component. This generalized component, not
part of the public API, can apply consistent design patterns to various
input element types.
@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Merging #827 (8160c6c) into main (74a5a25) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #827   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           56        57    +1     
  Lines          682       686    +4     
  Branches       240       241    +1     
=========================================
+ Hits           682       686    +4     
Impacted Files Coverage Δ
src/components/input/Input.tsx 100.00% <100.00%> (ø)
src/components/input/InputRoot.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

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

LGTM

@lyzadanger lyzadanger merged commit 9c1fbaa into main Feb 8, 2023
@lyzadanger lyzadanger deleted the input-root branch February 8, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants