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

[TextField] Add outlined and filled variants #12076

Merged
merged 2 commits into from
Sep 16, 2018
Merged

[TextField] Add outlined and filled variants #12076

merged 2 commits into from
Sep 16, 2018

Conversation

enagy27
Copy link
Contributor

@enagy27 enagy27 commented Jul 6, 2018

@Skaronator

This comment has been minimized.

@enagy27

This comment has been minimized.

@Skaronator

This comment has been minimized.

@enagy27

This comment has been minimized.

@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@mbrookes mbrookes added component: text field This is the name of the generic UI component, not the React module! new feature New feature or request labels Jul 7, 2018
@enagy27

This comment has been minimized.

@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@enagy27 enagy27 closed this Jul 9, 2018
@enagy27 enagy27 reopened this Jul 9, 2018
@enagy27

This comment has been minimized.

Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

Just a quick look.

display: 'inline-flex',
boxSizing: 'border-box',

borderStyle: 'solid',
Copy link
Member

Choose a reason for hiding this comment

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

Why a second border? Isn't the SVG providing this?

Copy link

Choose a reason for hiding this comment

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

In the spec, multiline is shown without a notch. I believe MCW also reverts to a border because it resizes appropriately as a non-absolutely positioned element. I could not find a way to get a responsive svg in this way, though it would be preferable

Copy link
Member

Choose a reason for hiding this comment

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

Odd, I can't see an example of multi-line textfield in the spec, only filled. However MCW haven't implemented filled or outline multiline, only textarea, which is technically something different.

minHeight: '60px',

// Match the notched outline
transition: theme.transitions.create(['borderColor', 'borderWidth'], {
Copy link

Choose a reason for hiding this comment

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

Yes, I have fixed this in my next commit by animating stroke and border, as opposed to the sub properties

/**
* If `true`, the TextField will be outlined.
*/
outlined: PropTypes.bool,
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be a variant prop to support other variants such as 'filled'

Copy link

Choose a reason for hiding this comment

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

Ok, perfect I’ll fix that

@@ -398,6 +479,7 @@ class Input extends React.Component {
onFocus,
onKeyDown,
onKeyUp,
outlined,
Copy link
Member

Choose a reason for hiding this comment

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

From context? (variant)

Copy link

Choose a reason for hiding this comment

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

It is, however I have applied it to context by reading props from the input, so that props such as this are applied to the input which seems to make the most sense, but are available in context

Copy link
Member

Choose a reason for hiding this comment

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

variant is a TextField prop, so it perhaps makes more sense to pass that down to FormControl, and add it to context there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that solution suitable for cases where consumers are not using TextField? In the current TextField implementation, variant will be passed into InputComponent and then grabbed from context. It's roundabout, but might that make sense for someone consuming Input and FormControl?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is outline needed on FormControl? It doesn't seem to do anything

minHeight: '60px',

// Match the notched outline
transition: theme.transitions.create(['borderColor', 'borderWidth'], {
Copy link

Choose a reason for hiding this comment

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

Yes, I have fixed this in my next commit by animating stroke and border, as opposed to the sub properties

multiline: {},
outlined: {
'&$multiline': {
backgroundColor: theme.palette.background.paper,
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 bit is wonky. In MCW the label background is hard coded to #FFF. I've instead moved that styling to the root and allowed the label to inherit. Ideally the scroll window would be sized appropriately, but I cannot figure out how to do so reliably on this field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a mistake- this should have been applied to the input. Unfortunately that means setting white as the default in two places

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe the label should have a background color. If you're referring to their Textarea, it's broken:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I absolutely agree. I'm not sure how to solve for this as the input is sized to the outline. This is also causing issues with dynamically resized multiline behaviors. Additionally, their use of a border on the div and the textarea is causing some artifacting with the inset. Any ideas?

@enagy27

This comment has been minimized.

@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@enagy27

This comment has been minimized.

@enagy27

This comment has been minimized.

@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@mbrookes mbrookes changed the title Outlined and Filled TextField Variants [TextField] Add outlined and filled variants Sep 7, 2018
@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@enagy27

This comment has been minimized.

@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@oliviertassinari oliviertassinari added this to the v3.x milestone Sep 9, 2018
@mbrookes

This comment has been minimized.

@enagy27

This comment has been minimized.

@enagy27

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@enagy27
Copy link
Contributor Author

enagy27 commented Sep 11, 2018

Absolutely. Let me know if there's anything further you need from me 😃

enagy27 and others added 2 commits September 16, 2018 11:23
…styling

Added examples to text fields doc examples

Removed unused state

Fixed broken tests after moving structure to RootRefs and added TypeScript definitions

Fixed regression on label due to lack of adorned start and isfilled context from form control. Moved outlined text fields to separate section.

Migrated form internals to extend react component to expose refs needed for the form control. Added notes to eslint-disable to explain why this was done.

Made a lot of progress aligning outlined text fields to the spec. Changed multiline styles not to use notched outline. Added hover state to form control context

Cleaned up styling around borders and highlighting. Added rtl theming for notched outline

Added polish. Animations applied for changing label color and border width (stroke width)

Moved outlined prop to variant prop. Added background to fit MCW styling and to present label on top of input text

Fixed multiline border behaviors. Removed hover state for multiline outlined. Added inset border with lower border radius. Added extra bottom border.

Converted to notched outline for outlined text fields using ResizeObserver

Got flexible multiline outlined variant text area working by fixing sizing issue where inline styles for height are applied to the root as opposed to the main text area

Forced hover to false on focus lost for select edge case. Added variant to selec
t and native select. Added styling to select and native select to reflect outlin
ed variant. Centered content within outlined select.

Small tweaks to get back to material specifications

Adjusted input height and dense margins.

Adjusted dense stylings

Fixed label transform on dense multiline input

All tests are passing. Changed usages of outlined to variant. Components such as InputLabel and FormHelperText which require multiline or variant info will now have these passed in as either context or as props.

Fixed yarn.lock issue.

Updated API docs

Fixed server lint errors

Bumped size limit.

Added some test cases for text field. Working through tests for notched outline and input still.

Updated docs:api

Added tests for input and notched outline controls

Fixed test failure on headless chrome

Fixed unmount issue in browser testing

Fixed broken test

Added animation for stroke width as well as color. Removed resize handle from multiline.

Cleaned up code in notched outline.

Separated out paths in outline to make them easier to reason about and to animate.

Animated the outline notch.

Fixed broken rtl outline, updated api docs, and updated tests.

Fixed regression due to improper styling override.

Adjusted styling to make svg growth appear inward.

Fixed tests for notched outline svgs.

Added tests for outline focused state.

Fixed select controls by giving them an svg fill instead of the input background color.

Updated docs to reflect new CSS API documentation

Updated standard dimensions of outlined input to be 280x56px. Modified notched outline svg shape tests accordingly.

Fixed label placement to more closely align visually with specifications.

Addressed PR feedback. Reverted to stateless functional components for Select, FormLabel, and FormHelperText. Moved away from getters and indirection to inline logic. Condensed svg path formatting.

Cleaned up svg output

Lowered size limit

Updated size limit

Fixed start and end adornments in outlined variant. Added examples in the docs.

Fixed lint errors and increased size limit

Fixed menu size in outlined text fields to be 280px

Upped size limit

Fixed webkit autofill overrun of the outline and label

Added borderRadius to outlined input to account for backgroundColor styling

Moved NotchedOutline out to separate folder. Added css documentation and exposed styling via NotchedOutlineProps. Added reference to NotchedOutline in TextField documentation. Changed functional style of NotchedOutline to match existing components.

Fixed input label overrun on chrome autofill

Fixed iOS rendering issue due to negative stroke dash offset

Fixed tests from changing dash offset from negative to positive
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@enagy27 Thank you for working on this pull-request. It was definitely a challenging task. I think that you did an awesome job 🔥❤️!

@mbrookes @Skaronator @enagy-earnup Thank you for the reviews.

I have made some changes to the pull-request. The most important one is regarding the component structure. I have introduced an InputBase component, same as ButtonBase, it's "unstyled".

                      +---------------+                   
                      |               |                   
                      |  InputBase    |                   
                      |               |                   
                      +---------------+                   
                     ---/     |      ---\                 
                 ---/         |          ---\             
   +--------------+   +---------------+   +--------------+
   |              |   |               |   |              |
   |  Input       |   | OutlinedInput |   | FilledInput  |
   |              |   |               |   |              |
   +--------------+   +---------------+   +--------------+                                                    

This way, people can build advanced overrides on top of the input without paying the overhead of all our variants (we have a Bootstrap UI demo demonstrating the approach, the TablePagination uses it too). I don't think that it's introducing any breaking change. I have created a ticket on my internal dashboard to simplify the implementation in the next breaking change release (v4).

@oliviertassinari
Copy link
Member

I'm expecting people to open issues regarding the new variants once we release them. It's a major undertaking. Let's hope for the best 🤞🍀.

@oliviertassinari oliviertassinari merged commit 8b5cd6a into mui:master Sep 16, 2018
@enagy27 enagy27 deleted the outlined-textfield branch September 16, 2018 19:48
@mbrookes
Copy link
Member

@enagy27 Thanks for the solid contribution, and for your perseverance given all the back and forth to arrive at the best approach!

@oliviertassinari

I'm expecting people to open issues

The only small issue I can see right now is the height of the select text fields, as mentioned in chat. But as you say, let's see! Nice job on the InputBase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants