fix(form-field): update label gap for outline style within the ngZone #12555
Conversation
googlebot
added
the
cla: yes
mmalerba
added
pr: lgtm
action: merge
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
josephperrott
force-pushed
the
form-field-outline
branch
3 times, most recently
from
bfab50d to
1ffd52f
Compare
josephperrott
force-pushed
the
form-field-outline
branch
3 times, most recently
from
4609d33 to
3f84826
Compare
src/lib/form-field/form-field.ts
Outdated
|
|
||
| /** | ||
| * @deprecated | ||
| * @breaking-change 7.0.0 | ||
| */ | ||
| @ViewChild('underline') underlineRef: ElementRef; | ||
|
|
||
| @ViewChild('connectionContainer') _connectionContainerRef: ElementRef; | ||
| @ViewChild('connectionContainer') _connectionContainerRef: ElementRef<HTMLDivElement>; |
There was a problem hiding this comment.
Since the connectionContainerattribute is on a div we can be sure that it will be ElementRef<HTMLDivElement>, we should be exact correct?
There was a problem hiding this comment.
You're not using any methods/properties specific to a div and I should be able to change the div to a span, or some other element without needing to change the logic. So to me that indicates that we should only require it to be an HTMLElement
src/lib/form-field/form-field.ts
Outdated
| Promise.resolve().then(() => this.updateOutlineGap()); | ||
| } | ||
| } | ||
| this.updateOutlineGap(); |
There was a problem hiding this comment.
I think we want that initialGapCalculated check so that we don't wind up measuring elements on every change detection
src/lib/form-field/form-field.ts
Outdated
| let startWidth = 0; | ||
| let gapWidth = 0; | ||
| const startEls = this._connectionContainerRef.nativeElement.querySelectorAll<HTMLDivElement>( | ||
| '.mat-form-field-outline > .mat-form-field-outline-start'); |
There was a problem hiding this comment.
Do we need a selector this complex? why not just .mat-form-field-outline-start?
josephperrott
force-pushed
the
form-field-outline
branch
5 times, most recently
from
de51165 to
1871760
Compare
src/lib/form-field/form-field.ts
Outdated
| // outline elements to show in the dom. This is a safe action to take because | ||
| // updateOutlineGap does not cause a change detection cycle to run. setTimeout is used | ||
| // because the update needs to be in a a new task rather than the next microtask. | ||
| setTimeout(() => this.updateOutlineGap()); |
There was a problem hiding this comment.
does this._ngZone.onStable.pipe(take(1)).subscribe(...) work for this? You should also kick it out of the zone
josephperrott
force-pushed
the
form-field-outline
branch
from
1871760 to
1ba1a39
Compare
josephperrott
force-pushed
the
form-field-outline
branch
from
1ba1a39 to
7177637
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #12353 #12457 #11577