feat(toHaveCss) Overload toHaveCSS matcher to accept React.CSSProperties #38617
Conversation
|
|
||
| CSS property value. | ||
|
|
||
| ### param: LocatorAssertions.toHaveCSS.styles |
There was a problem hiding this comment.
You need to update value parameter's type to accept React.CSSProperties rather than add a new parameter.
There was a problem hiding this comment.
What makes this a little awkward is that this feature requires less parameters than before. ie. one parameter for the pojo CSS Properties, as opposed to the two parameters for the name and the value.
The current PR handles multiple possible inputs for the first argument (either the name string, or the React.CSSProperties), albeit with some questionable naming.
Should I just delete the overload signatures, and use the main function definition that has union parameters?
There was a problem hiding this comment.
Sorry, I was not clear. What I meant is you can use function overload with #1 and #2 suffixes. See ## method: TestInfo.fixme#1 for example. Then first overload will stay as is and the second one will have CSSProperty as the only parameter. Then in the documentation we'd have 2 entries, one per overload (see e.g. https://playwright.dev/docs/api/class-testinfo#test-info-fixme-1), you'd need to make sure that there is proper usage section that presents both signatures. Support of overloaded methods is a bit messy in our API, but CSSPropery is not much different than other overloads, so it should work.
There was a problem hiding this comment.
Got it. I kept the existing implementation untouched as the base method, and added the new method definition as #2. This way, the other documentation that references toHaveCSS can remain unchanged.
A couple questions:
-
I did not add any usage related to non-JS APIs. Is that okay? Given the new CSSProperties parameter type, I wasn't sure how to translate them to different languages.
-
I set since to
v1.58. Is that right?
| import type { TestStepInfoImpl } from '../worker/testInfo'; | ||
| import type { APIResponse, Locator, Frame, Page } from 'playwright-core'; | ||
| import type { FrameExpectParams } from 'playwright-core/lib/client/types'; | ||
| import type { CSSProperties } from 'react'; |
There was a problem hiding this comment.
We can't afford a dependency on 'react'. Can you make it work in a best effort fashion similar to what we do for Electron types today? Basically, if the user has 'react' in their project, strong typecheck would apply. Otherwise accepted type should be string only.
There was a problem hiding this comment.
I added a type that uses 'react' only if it is imported correctly. If not, it uses a fallback type that has string keys.
Does this look okay? Also, is this the correct place to define this. I originally defined/exported this from matchers.ts, but had trouble getting the import paths to resolve.
Test results for "tests 1"32 failed 2 flaky34369 passed, 688 skipped Merge workflow run. |
- revert existing method definition to be the base definition. Add definition#2 that takes CSSProperties - Define CSSProperties type that is React.CSSProperties if react is installed. If not, defaults to an object with string keys
iumehara
force-pushed
the
35113-toHaveCSS-reactCSS-overload
branch
from
2d3bb1a to
02b8374
Compare
2 participants
References #35113
overrides-test.d.tsto include new signature