<Input /> v2 #47

juanca · 2019-07-26T20:30:38Z

Type of work: component

Deployment URL: https://mavenlink.github.io/design-system/input

(Optional for outside contributions) Tracked work in Mavenlink: https://www.pivotaltracker.com/story/show/165360149

Motivation

In order to better support MavenPages, we need an input component that is already accessible and forward-looking with design.

Version 2 of the input will bake in better accessibility by enforcing a label. It will also remove the pass-through properties interface to avoid consumer applications from misusing the input -- for example, we have a case where a developer used the input as a checkbox.

Misuses of components will create a maintenance burden in the future and misdirect vital demand information about MDS.

Useful resources:

Acceptance Criteria

It looks great in our support browsers list
It is backwards compatible with existing usages in Mavenlink (or usages are changed for more appropriate inputs)

Change log entry

Implement <Input> version 2. More accessible and better defined property interface.
Improve <Button>: allow name and value properties.

The pass-through technique might not be desirable in a library. It can create situations that make it hard to deprecate unforeseen usages of a given component. We should try to stick to supporting a minimal set of components which provide the look and feel of Mavenlink -- not an end-all developer tool.

… validation

src/components/input/input.jsx

This ensures we are always accessible and aligns with our design vision moving forward. Simplifies how to use the <Input> and thus minimizes our maintenance risk for the future.

…em-partII' into input

… test -u` or `yarn test --update` It is part of the docs.

juanca · 2019-07-31T23:37:37Z

Closes #34

…guring host applications

…ut it. Add disabled example for us to implement and style later.

Avoids the icon overflowing the input when the label is too large. Signed-off-by: Rob Levin <[email protected]>

…t that excludes node_modules

Signed-off-by: Rob Levin <[email protected]>

Remove unnecessary <div> container on <Input>

Signed-off-by: Rob Levin <[email protected]>

roblevintennis

@juanca I've added several questions and comments on the input api—I know you've mentioned you'll be perusing PRs while in your travels, but I'm not sure how often. Since this is meant to set a precedent from your email to me:

The button work and input work have set an interesting precedent of these.

I think my questions are important to resolve before moving on to new things e.g. the file input component...

Also, I see the base branch is all-svgs-in-internal-design-system-partII, so I suppose that's meant to reinforce the relationship? and/or was your thought that since you'd be away, I should consider just merging it in to my work since we weren't able to get the <Input /> fully reviewed and merged before you left? I'm uncertain whether you considered this WIP or dev done.

src/components/input/input.jsx

src/components/input/input.css

src/components/input/input.jsx

src/components/input/input.md

Otherwise, we add yet another requirement on host applications when using this library.

We want other to use each individual component. This makes it easier for static analysis. _Maybe_ in the future we will reverse this decision.

Conflicts: src/components/button/button.jsx src/components/button/button.test.jsx

We want to only support text-like types. We might as well do it little by little. Otherwise, we could get into a situation where someone uses the input as a checkbox!

…, Input#readOnly

…t flows with its container. Ideally, usages of MDS Input should provide their own layout -- or possibly a cssContainer class name with a width.

.eslintrc.js

README.md

jest.config.js

setup-enzyme.js

Bloodyaugust · 2019-11-08T22:37:02Z

src/components/input/input.jsx

+      </label>
+      <div className={props.cssContainer}>
+        <input
+          className={getClassName(props.className, props.invalid)}


I'm concerned about the number of configurations here. Are there things we could do, like advocate for the usage of native handlers for some of these?

This is an opinionated implementation.

As you can see from the code removed section, the MDS Input v1 was just that -- it passed through all props onto the <input> element. We thought that would be the best for MDS library but we saw incorrect usages of our MDS Input. Since we can just pass any kind of type into the MDS input, we ended up in a situation where a feature team used our MDS input as a checkbox -- which basically gutted the whole thing.

Now, I think I understand the whole battle between configuration vs composition. While we do see many configuration options, I do not think this is a "configurable" component -- I think that is usually applied to components that change their behavior based on their properties. In this case, we are whitelisting the configuration options to the <input> -- which means we want to control how much configuration is allowed for native elements.

Time to have another look at typescript :) We're having same sorts of nuanced issues at my new place and have even gone so far as to throw a fatal Error if unrecognized props are passed in—this starts to make the code less readable and it's not fault tolerant per se. Typescript lets you know at compile time, you can just declare the interface you expect (prop-types is really not so great at enforcing and devs will just ignore warnings whereas an interface will be noisy at compile time and not let the application developers use incorrect props at all). It IS a PITA to configure TS at times though.

Something to maybe consider longer term I dunno :)

I'm definitely not comfortable with the idea that implementation issues should be solved by just switching languages, there's a wealth of baggage for any language. 😄 I do like the opinionated approach, but perhaps we could forward native event handlers specifically? That way our interface maintains at least part of the underlying HTML native interface, and the part that we need for proper usage of the component while also supporting wrappers for more advanced behavior.

There are no native event handles in React! React uses its own event system (and, sure, it ties it into native event handles but that is abstracted away from us React developers).

@roblevintennis Hello! 👋

Yeah, I'm on the same boat as always: I don't want to adopt yet another non-standard thing if I can post-pone it as long as possible. We adopted React and CSS Modules but we did wait a good 4 years before doing any of that. Hah, maybe in 4 years I'll consider TS. ;)

I feel ya kinda ;) In this spirit of coding towards standards, I'd be very curious if esnext et al introduce some sort of notion of interfaces; although it's sort of useless as a runtime only thing imo (main thing is to get compile / transpile knowledge of bad inputs).

If you think about it, really, Webpack is a very specific way to set things up and many folks consider it extremely painful (folks at my new gig are in favor of using create react app or gatsby or something because they've tired of fighting Webpack). React leaves itself around in your code (e.g. read the philosophy of Svelte which compiles itself away). CSS Modules is really just a nifty spec which ala css-loader and Webpack does some name munging tricks, and as we know does not work perfectly (although I'm still using it as it's the least of all unfortunate options).

All to say, our jobs and lives in web-land are still a mess and not really so standards-based 😋 I am sad that my new gig is still quite rooted in Sass but maybe we'll get on the PostCSS train in 2020.

there's a wealth of baggage for any language

Typescript is not really a language but more of a superset of JavaScript. Yes, there's some baggage, but also some benefits. Especially if you like to use inversion of control (which I know you loathe from a particular meeting I recall where we brought up DI and you were a bigtime detractor) @Bloodyaugust :)

src/components/input/input.jsx

Bloodyaugust

LGTM! We'll keep an eye on component complexity going forward.

…ajor version 0.

juanca added 8 commits July 25, 2019 15:52

Remove private documentation from source files

cf7a7f4

Refactor: destructure props on its own expression

dbe66c4

Remove redundant comment -- this is made evident by the property type…

7acd05d

… validation

Whoops: implement and test placeholder API

5a8eb52

Refactor Jest configuration into own file

46ebd24

Set verbose output with Jest

9e38540

Ensure Jest fails when prop-types are invalid

a7c0aff

juanca commented Jul 26, 2019

View reviewed changes

src/components/input/input.jsx Outdated Show resolved Hide resolved

juanca added 18 commits July 30, 2019 11:56

Implement label on input component

b1a89e0

This ensures we are always accessible and aligns with our design vision moving forward. Simplifies how to use the <Input> and thus minimizes our maintenance risk for the future.

Implement required prop interface

6ec88e2

Merge remote-tracking branch 'origin/all-svgs-in-internal-design-syst…

3e0aa61

…em-partII' into input

Improve <Input> documentation: add required example

7f41cfe

Implement invalid API

a118453

ESLint fixes

8a76f86

Remove redundant Yarn script: yarn test:update is the same as `yarn…

84a1eba

… test -u` or `yarn test --update` It is part of the docs.

Remove deprecated Yarn script

508f180

Fix trailing comma in JSON file

aef5021

Update snapshot to reflect legacy way of adding for on <label>

b68c2bc

Remove unnecessary option on babel-loader

9963640

Document how to process JSX via Babel

a92af47

Document how to process CSS modules

0a2d7ea

Consistent syntax

29217f9

Typo: .

788c5d2

Document how to process SVGs

5c6fd90

Update README: correct import statement for Input component

cd74308

Tidy up markdown

e2db042

juanca changed the base branch from master to all-svgs-in-internal-design-system-partII July 31, 2019 23:40

Ensures we are always explicit about extensions to avoid overly confi…

e98fbe6

…guring host applications

juanca and others added 7 commits August 2, 2019 14:19

Remove required example since there is nothing visually different abo…

98c85e2

…ut it. Add disabled example for us to implement and style later.

Ensure caution icon is always at the top-right of the input.

ceb6b15

Avoids the icon overflowing the input when the label is too large. Signed-off-by: Rob Levin <[email protected]>

Remove redundant Webpack config: include already filters down the lis…

2c0b7e5

…t that excludes node_modules

Implement styling on the label when the component is invalid

b4c7b15

Signed-off-by: Rob Levin <[email protected]>

Implement disabled API on input

84d2c06

Signed-off-by: Rob Levin <[email protected]>

Update specs to reflect wrapping <div> on <input>

ab9040f

Remove unnecessary <div> container on <Input>

ESLint fixes and Stylelint fixes

25083b2

Signed-off-by: Rob Levin <[email protected]>

roblevintennis suggested changes Aug 7, 2019

View reviewed changes

src/components/input/input.jsx Show resolved Hide resolved

src/components/input/input.css Show resolved Hide resolved

src/components/input/input.jsx Show resolved Hide resolved

src/components/input/input.jsx Show resolved Hide resolved

src/components/input/input.jsx Show resolved Hide resolved

roblevintennis reviewed Aug 7, 2019

View reviewed changes

src/components/input/input.md Show resolved Hide resolved

roblevintennis approved these changes Aug 10, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Merge branch 'master' into input

ed636d0

juanca changed the base branch from all-svgs-in-internal-design-system-partII to master August 23, 2019 19:10

juanca and others added 7 commits August 23, 2019 12:12

ESLint fixes: require extensions

a61d7fd

Otherwise, we add yet another requirement on host applications when using this library.

Deprecate MDS entry API for Input

ef902d2

We want other to use each individual component. This makes it easier for static analysis. _Maybe_ in the future we will reverse this decision.

Merge branch 'input' of github.com:mavenlink/design-system into input

33170be

Merge branch 'master' into input

be75432

Conflicts: src/components/button/button.jsx src/components/button/button.test.jsx

Implement Input#type property interface as a set of valid types

39cb426

We want to only support text-like types. We might as well do it little by little. Otherwise, we could get into a situation where someone uses the input as a checkbox!

Implement Input#maxLength property interface

9acffc7

Implement Input#onBlur, Input#onFocus, Input#onInput, Input#onKeyDown…

148def9

…, Input#readOnly

juanca requested a review from Bloodyaugust November 5, 2019 03:15

juanca added 2 commits November 7, 2019 10:56

Ensure the container flows with its parent container. Ensure the inpu…

68ba883

…t flows with its container. Ideally, usages of MDS Input should provide their own layout -- or possibly a cssContainer class name with a width.

Backfill cssContainer and cssLabel tests

eee96d8

This comment has been minimized.

Sign in to view

ES Lint fix

a1f8012

Bloodyaugust reviewed Nov 8, 2019

View reviewed changes

Bloodyaugust approved these changes Nov 13, 2019

View reviewed changes

Bump minor version -- this is a breaking change but we are still in m…

a564019

…ajor version 0.

juanca merged commit 097d8ec into master Nov 13, 2019

juanca deleted the input branch November 13, 2019 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

<Input /> v2 #47

<Input /> v2 #47

juanca commented Jul 26, 2019 •

edited

Loading

juanca commented Jul 31, 2019

roblevintennis left a comment

This comment has been minimized.

This comment has been minimized.

Bloodyaugust Nov 8, 2019

juanca Nov 13, 2019

roblevintennis Nov 13, 2019 •

edited

Loading

Bloodyaugust Nov 13, 2019

juanca Nov 13, 2019

juanca Nov 13, 2019

roblevintennis Nov 13, 2019

roblevintennis Nov 13, 2019 •

edited

Loading

Bloodyaugust left a comment

<Input /> v2 #47

<Input /> v2 #47

Conversation

juanca commented Jul 26, 2019 • edited Loading

Motivation

Acceptance Criteria

Change log entry

juanca commented Jul 31, 2019

roblevintennis left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Bloodyaugust Nov 8, 2019

Choose a reason for hiding this comment

juanca Nov 13, 2019

Choose a reason for hiding this comment

roblevintennis Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Bloodyaugust Nov 13, 2019

Choose a reason for hiding this comment

juanca Nov 13, 2019

Choose a reason for hiding this comment

juanca Nov 13, 2019

Choose a reason for hiding this comment

roblevintennis Nov 13, 2019

Choose a reason for hiding this comment

roblevintennis Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Bloodyaugust left a comment

Choose a reason for hiding this comment

juanca commented Jul 26, 2019 •

edited

Loading

roblevintennis Nov 13, 2019 •

edited

Loading

roblevintennis Nov 13, 2019 •

edited

Loading