refactor(form-field/testing): deprecate methods that return TestElement #19940
Conversation
crisbeto
added
P3
googlebot
added
the
cla: yes
| /** | ||
| * Gets a reference to the container element which contains all projected | ||
| * suffixes of the form-field. | ||
| * @deprecated Use `getSuffixText` instead. |
There was a problem hiding this comment.
Should we actually just fix this? Seems like this happened by mistake, but the methods actually should exist?
There was a problem hiding this comment.
The reason I think we didn't do <..>Text is that a prefix/suffix doesn't necessarily need to be about text. i.e. it can contain icons or arbitrary content, which can be tested against with the harness loader too?
There was a problem hiding this comment.
Oh, looks like this has been changed in 1c1af58 as part of Miles' suggestion (#17874 (comment)) but the logic has not been updated properly. Any reason to deprecate the actual method instead of making it return a harness loader?
There was a problem hiding this comment.
I decided to add the text method since I don't see what else you would be doing with the TestElement. We could make it return a harness loader, but at this point it'll be a breaking change.
There was a problem hiding this comment.
Yeah for sure, it shouldn't be a TestElement. It just feels wrong to me to deprecate the method while we actually would want to keep the method (just with the fix IMO). cc. @mmalerba for thoughts on this.
The additional methods you added are a nice addition we could keep anyway I guess.
There was a problem hiding this comment.
@crisbeto Chatted with Miles. Looks like we can deprecate this as we'll add the more generic harness loader methods w/ an enum for accessing the prefix/suffix containers. That makes sense to me.
In general we want to avoid exposing `TestElement` instances in the public harness API. These changes mark `getHarnessLoaderForSuffix` and `getHarnessLoaderForPrefix` as deprecated since they don't do what their names say and they're exposing a `TestElement`. I've added replacements that return the text of the elements since that's what appears to be the main use case.
crisbeto
force-pushed
the
COMP-363/form-field-harness-test-element
branch
from
9b135b8 to
ef5e190
Compare
|
To summarize the various discussions:
Given the points above, I'll keep the changes as they are. |
…nt (#19940) In general we want to avoid exposing `TestElement` instances in the public harness API. These changes mark `getHarnessLoaderForSuffix` and `getHarnessLoaderForPrefix` as deprecated since they don't do what their names say and they're exposing a `TestElement`. I've added replacements that return the text of the elements since that's what appears to be the main use case.
|
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. |
In general we want to avoid exposing
TestElementinstances in the public harness API. These changes markgetHarnessLoaderForSuffixandgetHarnessLoaderForPrefixas deprecated since they don't do what their names say and they're exposing aTestElement. I've added replacements that return the text of the elements since that's what appears to be the main use case.Note: marking as merge safe, because it only adds new APIs without touching existing behavior.