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

fix(cdk/text-field): autosize text areas using the placeholder #22314

Merged
merged 5 commits into from
Apr 2, 2021
Merged

fix(cdk/text-field): autosize text areas using the placeholder #22314

merged 5 commits into from
Apr 2, 2021

Conversation

mmalerba
Copy link
Contributor

Roll-forward w/ fix for #22197

This reverts commit 4316787.

@mmalerba mmalerba added P2 The issue is important to a large percentage of users, with a workaround G This is is related to a Google internal issue target: major This PR is targeted for the next major release area: cdk/text-field labels Mar 22, 2021
@mmalerba mmalerba requested a review from a team as a code owner March 22, 2021 22:13
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 22, 2021
crisbeto
crisbeto approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM once the CI failures have been resolved.

@mmalerba mmalerba mentioned this pull request Mar 23, 2021
Closed
devversion
devversion approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM

@mmalerba
Copy link
Contributor Author

After looking into the CI failures, I've discovered that this change conflicts with some logic in MatInput that deletes the placeholder to prevent screen readers from reading it out twice. According to the comment it's only there to support the legacy form-field, so we should probably just remove it. https://github.com/angular/components/blob/master/src/material/input/input.ts#L370-L380

I'm going to mark this PR as blocked pending that work

@mmalerba mmalerba added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Mar 23, 2021
@mmalerba
Copy link
Contributor Author

I've created a PR to remove the legacy placeholder logic #22319

@mmalerba
Copy link
Contributor Author

Alternatively, if we don't care about the autosize behavior with placeholder being a little buggy in the legacy appearance, I can just update the test to use a different form-field appearance. (Made a commit with that change, CI should go green 🤞

@mmalerba mmalerba mentioned this pull request Mar 24, 2021
Closed
@mmalerba mmalerba removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Mar 25, 2021
@mmalerba
Copy link
Contributor Author

After some discussion I think its best we just implement this fix for the standard, fill, and outline appearances, not the legacy appearance

crisbeto
crisbeto approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Mar 31, 2021
@annieyw annieyw merged commit 2427abe into angular:master Apr 2, 2021
@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 May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto approved these changes

@devversion devversion devversion approved these changes

Assignees

@andrewseguin andrewseguin

Labels
action: merge The PR is ready for merge by the caretaker area: cdk/text-field cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mmalerba @crisbeto @devversion @annieyw @andrewseguin