URO-209: add additional content and tests to orcidlink #215
Conversation
multilple out-of-date transitiive dependency upon testing-library/dom leads to errors with user-event. see testing-library/user-event#1104 (comment)
wip - it doesn't actually link yet
and add an image for content
and some test mock additions and changes
replace structuredClone with jsonCopy as core-js is not directly specified in package.json
|
have a couple more commits, but pre-commit is misbehaving, working on it ... |
store in regularly named directory (per-feature), clean up url constants
|
See https://docs.google.com/document/d/1LhfEjrg78LcmGZhf4dtrGqcojwdGZoIMs0SUvR_d_mE/edit#heading=h.86bd2vg2e642 for screenshots of before and after. |
|
Apologies for the delay. Had an unproductive week last week. I have a review pending for this but I'm just poking at the dependency override, as that isn't ideal. |
src/features/orcidlink/test/mocks.ts
Outdated
| /** | ||
| * Make a copy of a given "json-compatible" value. | ||
| * | ||
| * This can be replaced with "structuredClone". In order to that that, we need | ||
| * to use core-js, but we don't have a direct dependency upon core-js; it is a | ||
| * transitive dependency of several packages already. | ||
| * | ||
| * This technique is not ideal, but in the limited usage of it for test data, | ||
| * works just fine. | ||
| */ | ||
| export function jsonCopy<T>(value: T): T { | ||
| return JSON.parse(JSON.stringify(value)); | ||
| } |
There was a problem hiding this comment.
We can make core-js a dev dependency as it would only run in the tests, that should mean it doesn't affect the build size/deps
There was a problem hiding this comment.
Hi @dauglyon , was this meant to be a request for change, or to record a comment for future improvement?
src/features/orcidlink/test/mocks.ts
Outdated
| return JSON.parse(JSON.stringify(value)); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
I believe we have a noOp util func already
|
Hey @dauglyon, I'm still catching up after being out. I'm wondering whether you meant the comments above to be requests for change, or is this ready for merging. |
|
You're good to merge, both were just minor suggestions |
|
@dauglyon Great, thanks. I went ahead and implemented those changes anyway. noop was an oversight - i had searched case sensitively for noop so didn't see the noOp; as for copyJSON, thought that adding a line liner test function was better than a new dependency - and it gets confusing since structuredClone is considered by the dev environment to be available (since it is in node now and browsers for a couple years), but the ts config does not consider it available, and I didn't want to track that weirdness down ... anyway, adding the global import |
This set of changes implements URO-209, whose purpose is primarily adding additional content on views added in URO-208.
The changes include a few buttons, which do not invoke significant new functionality - just navigation and placeholder console logging (for testing.)
An addition was made to package.json, to pin
testing-library/dom, which is required to avoid test warnings/error mesages when using@testing-library/user-event. This is due to conflicting dependencies, which end up specifying multiple versions of testing-library/dom transitively, which causes the error message `Warning: The current testing environment is not configured to support act(...)' to be emitted during tests (yes, a message labeled as "Warning", emitted via console.error, which does not cause test failure.)One solution, taken here, is to use the advice in testing-library/user-event#1104 (comment), to pin
@testing-library/domto at least9.0.1(well, at least version 9).I don't think updating storybook would help, but it may, as it is two major versions behind. Issues at storybook point the finger at testing-library/user-event.
I tested storybook, and it works fine with the pinned testing-library/dom.