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

fix(material/form-field): outline label position #29138

Conversation

adumitrescu-plenty
Copy link
Contributor

When the form field is inside an overlay with ng-content or inside a side-nav (mat-drawer) the prefix container would not be visible thus the label would overlap with the prefix. In this fix we narrow down possible cases that would require an additional check for the prefix container width.

Example of overlay: https://stackblitz.com/edit/ipwssz-9pucs5?file=src%2Fexample%2Fcdk-overlay-basic-example.html
Example of side-nav: https://stackblitz.com/edit/ad4b8f-dq4pmf?file=src%2Fexample%2Fsidenav-autosize-example.html

@adumitrescu-plenty adumitrescu-plenty changed the title fix(material/form-field): label position fix(material/form-field): outline label position May 30, 2024
@adumitrescu-plenty
Copy link
Contributor Author

@crisbeto Can you have another look? Pretty please 😄

@devversion devversion removed their request for review June 10, 2024 10:33
@adumitrescu-plenty
Copy link
Contributor Author

@crisbeto Just a reminder :) I know you are busy but .. pretty please 😄

@adumitrescu-plenty adumitrescu-plenty requested a review from a team as a code owner June 14, 2024 08:51
@adumitrescu-plenty adumitrescu-plenty requested review from crisbeto and removed request for a team June 14, 2024 08:51
@crisbeto crisbeto removed the request for review from mmalerba June 18, 2024 12:56
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jun 18, 2024
@crisbeto
Copy link
Member

@adumitrescu-plenty can you remove the merge commit from the PR? Otherwise I can't merge the PR.

@adumitrescu-plenty adumitrescu-plenty force-pushed the fix/form_field/outline_label_when_prefix_not_visible branch from c8c7068 to 2c6befe Compare June 18, 2024 13:32
@angular-robot angular-robot bot requested a review from crisbeto June 18, 2024 13:32
When the form field is inside an overlay with ng-content or inside a side-nav (mat-drawer) the prefix container would not be visible thus the label would overlap with the prefix. In this fix we narrow down possible cases that would require an additional check for the prefix container width.
Add caching for shadow root so it doesn't have to be resolved each time
@crisbeto
Copy link
Member

It looks like there's a merge conflict now.

@adumitrescu-plenty adumitrescu-plenty force-pushed the fix/form_field/outline_label_when_prefix_not_visible branch from 2c6befe to 5648c0b Compare June 18, 2024 13:46
@crisbeto crisbeto added target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Jun 18, 2024
@crisbeto crisbeto merged commit 6318f24 into angular:main Jun 18, 2024
22 of 24 checks passed
crisbeto pushed a commit that referenced this pull request Jun 18, 2024
* fix(material/form-field): fix label position

When the form field is inside an overlay with ng-content or inside a side-nav (mat-drawer) the prefix container would not be visible thus the label would overlap with the prefix. In this fix we narrow down possible cases that would require an additional check for the prefix container width.

* perf(material/form-field): add caching for shadow root

Add caching for shadow root so it doesn't have to be resolved each time

(cherry picked from commit 6318f24)
@adumitrescu-plenty adumitrescu-plenty deleted the fix/form_field/outline_label_when_prefix_not_visible branch June 19, 2024 05:43
crisbeto added a commit to crisbeto/material2 that referenced this pull request Jun 19, 2024
@crisbeto
Copy link
Member

I'm really sorry, but I have roll this back again in #29282. It ended up breaking in a similar way to last time where there was a case when the form field is initially hidden and when it becomes visible, the label is overlapping the prefix.

@adumitrescu-plenty
Copy link
Contributor Author

Can you provide more details so I can have another deep look into it? Thank you

crisbeto added a commit that referenced this pull request Jun 19, 2024
crisbeto added a commit that referenced this pull request Jun 19, 2024
@crisbeto
Copy link
Member

I can't run the app that is failing, but I've seen the screenshot tests of the failure. It's a similar case to last time where a form field with a prefix is placed inside a sidenav that is hidden initially. Once the sidenav is opened, the label is rendered on top of the prefix. A difference compared to last time is that the form field seems to be nested in a bunch of other elements within the sidenav.

@adumitrescu-plenty
Copy link
Contributor Author

I will try to see what can I do to fix it :) I won't give up on this, sorry 😄

@adumitrescu-plenty
Copy link
Contributor Author

@crisbeto can you provide a structure or something ? stackblitz example or just plain code of that sidenav? I can't seem to make it brake myself. Thank you

@crisbeto
Copy link
Member

crisbeto commented Jun 19, 2024

I'm not allowed to share any code and it's a pretty complex app so narrowing it down is hard. It's basically a mat-form-field with an icon-based prefix and it's nested in several layers of components. It looks like the container that is hidden isn't a mat-sidenav, but a div that switches between display: none and display: block.

DBowen33 pushed a commit to DBowen33/components that referenced this pull request Jul 3, 2024
* fix(material/form-field): fix label position

When the form field is inside an overlay with ng-content or inside a side-nav (mat-drawer) the prefix container would not be visible thus the label would overlap with the prefix. In this fix we narrow down possible cases that would require an additional check for the prefix container width.

* perf(material/form-field): add caching for shadow root

Add caching for shadow root so it doesn't have to be resolved each time
DBowen33 pushed a commit to DBowen33/components that referenced this pull request Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants