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.

Sign up for GitHub

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

Jump to bottom

feat(material-experimental): initial prototype of mdc-form-field #17903

Merged
merged 5 commits into from
Jan 7, 2020
Merged

feat(material-experimental): initial prototype of mdc-form-field #17903

merged 5 commits into from
Jan 7, 2020

Conversation

devversion
Copy link
Member

@devversion devversion commented Dec 9, 2019 •
edited
Loading

Creates the initial prototype of form-field based on MDC text-field.

A few things that required some special handling in terms of extra CSS:

  • MDC text-field bottom-lines are limited to input and textarea. We want them to work always and to also cover prefixes and suffixes.
  • MDC text-field does not have a real standard appearance that works well for inputs and textarea's.
  • MDC text-field relies on special class being applied if textarea is the control. This doesn't match our model of "abstract" form field controls (so we needed custom handling; see SCSS comments)

Based on demo comparisons and unit test comparisons there are no breaking changes in terms of API and features, except that prefixes and suffixes do not work properly in the outline appearance. This requires some more thoughts and most likely upstream changes in MDC text-field. I could imagine this being something we can tackle in follow-ups's as the PR is already quite large.

@devversion devversion requested review from jelbourn, mmalerba and a team as code owners December 9, 2019 20:44
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 9, 2019
@devversion devversion force-pushed the feat/mdc-input branch 6 times, most recently from bb8d163 to 9bce312 Compare December 10, 2019 14:42
@devversion devversion added the new component This issue is tracking a new component to be added to the library label Dec 10, 2019
jelbourn
jelbourn reviewed Dec 12, 2019
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I got about halfway through before I have to head out for the day; will try to finish tomorrow

Can you file an issue on the MDC repo for the challenges with prefix/suffix? I know their team is planning on supporting this soon, so we should make our requirements known

cc @abhiomkar

@devversion
Copy link
Member Author

@jelbourn I created material-components/material-components-web#5326 for the outline appearance prefix/suffix issue (and linked to it in the Sass where needed)

jelbourn
jelbourn reviewed Dec 12, 2019
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

I noticed that the error text is a bit darker- is that intentional?
image

I also noticed that the text color inside a toolbar isn't contrasted; was that built in before or was it part of the demo?
image

src/material-experimental/mdc-form-field/form-field.ts
* Sets the list of element IDs that describe the child control. This allows the control to update
* its `aria-describedby` attribute accordingly.
*/
private _syncDescribedByIds() {
Copy link
Member

Choose a reason for hiding this comment

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

Would using AriaDescriber simplify this method at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. We want to use existing DOM elements and the actual custom form-field control is responsible for setting them. We just pass the hint/error id's down to the registered control.

mmalerba
mmalerba reviewed Dec 13, 2019
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

Still reviewing, but here's a couple comments I had so far

@devversion
Copy link
Member Author

For reference: Theming and colors are inaccurate since we do not have feature targeting yet. Without feature targeting I cannot wrap the MDC text-field styles in the theming mixin (limitation of Sass)

@devversion devversion force-pushed the feat/mdc-input branch 2 times, most recently from 631fd63 to c728167 Compare December 16, 2019 18:36
@devversion
Copy link
Member Author

@jelbourn @mmalerba I addressed all of the feedback. Can you please have another look?

mmalerba
mmalerba approved these changes Dec 16, 2019
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

I hope that we'll be able to make some changes in MDC over time to reduce the number of workaround needed, but this looks good to me as a first pass.

Also, amazing work on this! Thank you for the detailed comments and explanations!

jelbourn
jelbourn approved these changes Dec 16, 2019
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Your comments in this PR are great; they could be in a textbook about good comments.

@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Dec 16, 2019
@abhiomkar
Copy link

@jelbourn Thanks for bringing this up! I sent you a note internally for potential changes to text field for prefix / suffix support.

@mmalerba mmalerba merged commit 697c3a0 into angular:master Jan 7, 2020
yifange pushed a commit to yifange/components that referenced this pull request Jan 30, 2020
@devversion @yifange
feat(material-experimental): initial prototype of mdc-form-fie… (angu…
1b0fc8b 
…lar#17903)

* feat(material-experimental): initial prototype of mdc-form-field

Initial prototype of form-field based on MDC text-field.

* fixup! feat(material-experimental): initial prototype of mdc-form-field

Address feedback

* Use MDC text-field variables as much as possible.

* fixup! fixup! feat(material-experimental): initial prototype of mdc-form-field

Address feedback 2

* fixup! feat(material-experimental): initial prototype of mdc-form-field

Address feedback
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@mmalerba mmalerba mmalerba approved these changes

Assignees

@mmalerba mmalerba

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement new component This issue is tracking a new component to be added to the library target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devversion @abhiomkar @jelbourn @mmalerba @googlebot