Breadcrumbs refactor, testing using jest & storybook #413

mrabbitt · 2024-09-17T20:56:12Z

Included in this PR

Refactored BreadcrumbsNav into two components for better separation of concerns and testability:
- Stateful BreadcrumbTrail: Stateful container component that gets the URL path from and fetches student and para names via router calls. Passes data as props to BreadcrumbTrail.
- BreadcrumbTrail: Stateless component which handles all layout/rendering using given props.
Added basic "stories" files for BreadcrumbTrail which allows previewing and testing its appearance in various states using Storybook.
Added support to compass project for testing using jest
- Since ava is already used a test runner for the backend tests, separate scripts were added to package.json:
  - test:backend runs existing tests for backend code with ava
  - test:frontend runs new frontend tests using jest
  - test runs both backend and frontend tests
Added jest tests for Breadcrumbs:
- BreadcrumbTrail.test.tsx tests BreadcrumbTrail component using snapshot testing
- BreadcrumbTrailUtils.test.ts contains jest unit tests for a function used by BreadcrumbDesign to transform its input into breadcrumb segments.

Not Included in this PR

Fixes to breadcrumb behavior problems described in issue Breadcrumb on goal detail page links to broken /goals page, doesn't link back to student #396. I plan to address this in a later PR once some testing is in place.

What input is especially requested from reviewers:

Which test runner(s) do we want to use for compass? ➡️ Update: Based on feedback, we're going with option 1 (as implemented in this PR branch
- My thoughts: ava is a decent simple option for testing pure JavaScript/TypeScript code that has worked fine for backend tests. However, it's not as widely used and documented as jest, and I ran into significant problems trying to test components / JSX with it and was ultimately unsuccessful (will link to another draft PR with these attempts). By comparison, the process getting jest to integrate with NextJS and test components was well documented and worked without issue.
- Options:
  - Option 1: Use ava for backend and jest for frontend (what's currently implemented in this PR)
  - ~~Option 2: Use ava for all tests~~
    - This requires us to either drop the idea of unit testing or snapshot testing JSX code, or resolve the issues I ran into (link to ava draft PR)
    - (Note that the tests in transformBreadcrumbs.test.ts will work with ava without further research because they're testing pure TypeScript code without JSX.)
  - ~~Option 3: Use jest for all tests~~
    - This would require converting the existing backend tests to use jest instead of ava. (I haven't done any research into this, but am happy to try this if there's interest in this approach).~~
Do the the types of tests evaluated here make sense to write and maintain?
Suggestions on Breadcrumbs component/function naming ("BreadcrumbTrail", "StatefulBreadcrumbTrail" etc.)
- ~~I'd like to rename the component files to match the component names once we're happy with the names, but haven't done that yet.~~
- **Update: ** Names have been updated based on feedback as described in this comment.

Thank you for reading all this and for your input! 😃

trpc dependency not handled yet

…sNav.

(Reverted changes to package.json for storybook) # Conflicts: # package-lock.json # package.json # src/components/design_system/breadcrumbs/Breadcrumbs.tsx

…r inner display-only component.

…component

…son data.

katconnors · 2024-09-17T22:00:44Z

src/components/design_system/breadcrumbs/__snapshots__/Breadcrumbs.test.tsx.snap

@@ -0,0 +1,227 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`BreadcrumbsDesign renders empty for / 1`] = `


what is the "renders empty for / 1`" referring to?

This file is generated by Jest. Jest will generate these .snap files for test files with tests that use .toMatchSnapshot() if they don't already exist. If the component code is changed so that it no longer matches the snapshot, the test will fail. To update the snapshot files to match the current version of the component (assuming changes to component are intended), you run jest with the -u or --updateSnapshot option. The way our project is set up in this PR branch, you can update all snapshot files with:

npm run test:frontend -- --updateSnapshot

BreadcrumbsDesign renders empty for / is the name of the corresponding test in Breadcrumbs.test.tsx I'm not sure what the "1" refers to. As long as it makes sense to Jest, I guess that's what matters. 😄

See Jest docs for more on Snapshot testing: https://jestjs.io/docs/snapshot-testing

@katconnors once Jest support is merged, would it make sense to document some of this in the README file or compass project wiki? Where do you think would be the best place?

I just found this comment by doing a search for "readme"... I would definitely advocate for updating the README. My personal best guess would be to go into the h3 "Running Tests", put all the existing content into an h4 for "Backend Tests", add information for "Frontend Tests" as another h4 below it, and then lastly one more h4 for "All Tests". It might be good to also add a link to https://jestjs.io/docs/snapshot-testing, which is where I just went to try to remember how snapshot testing works.

Also, the number seems to refer to the number of the test. Just playing around with it now, I created another test "renders empty for /students/student-id" with different props, and the resulting test generated an additional snapshot, so now there's a

exports[BreadcrumbsDesign renders empty for /students/student-id 2] = `

snapshot in addition to the first one on my local branch version of the snapshot file.

katconnors

High level looks good to me, with the caveat that I haven't used jest myself.
Some other comments:
-I really appreciate how thorough the documentation is (both the overview and the file specific comments).
-functionality is good-the tests passed for me (front and backend)
-I would break into multiple PR's in the future (thousands of line changes is a very meaty PR!)

For your questions, I think it would be cool if we integrated some jest into the testing. Leaning towards not completely abolishing ava, but jest is really relevant for the industry and it's a good skill to have.

I don't have strong opinions on the naming, so others can chime in.

mrabbitt · 2024-09-20T01:27:11Z

.storybook/preview.tsx

+    /*
+      Attention: We've detected that you're using actions.argTypesRegex together with the visual test addon:
+
+      /Users/mike/Documents/coding/compass/.storybook/preview.tsx
+        9 | const preview: Preview = {
+        10 |   parameters: {
+      > 11 |     actions: { argTypesRegex: "^on[A-Z].*" },
+          |                ^^^^^^^^^^^^^
+        12 |     controls: {
+        13 |       matchers: {
+        14 |         color: /(background|color)$/i,
+
+      We recommend removing the argTypesRegex and assigning explicit action with the fn function from @storybook/test instead:
+      https://storybook.js.org/docs/essentials/actions#via-storybooktest-fn-spy-function
+    */
+    // actions: { argTypesRegex: "^on[A-Z].*" },


TODO: Take this out of this PR.

@nickvisut is going to take a look at this in a new issue.

mrabbitt · 2024-09-20T01:28:40Z

jest.setup.ts

+// eslint-disable-next-line @typescript-eslint/no-unsafe-return
+jest.mock("next/router", () => require("next-router-mock"));


TODO: Take this out; no longer mocking the router!

mrabbitt · 2024-09-20T01:49:32Z

package.json

    "lint-staged": "^13.1.0",
+    "next-router-mock": "^0.9.13",


TODO: Remove this

thomhickey · 2024-09-20T15:22:34Z

@mrabbitt

Awesome stuff here. Storybook is blowing my mind, what a huge time saver. Can't count on a million fingers how many times I've drilled into a UI just to see my latest changes of some internal component. HMR alleviates some of this, but storybook is a whole new level.
Let's go with Jest everywhere as far as I'm concerned. I'd be happy to help rewrite the ava tests into Jest. It's a shame to have two test runners. If not, then Jest front-end, Ava back-end is fine with me. The way you've integrated them both into package.json is great.
For names I suggest BreadcrumbTrail (parent) and Breadcrumb (child) for the two components - note singular not plural.
Love the snapshot testing.
I'm getting a lot of 'unsafe call of an any typed value' in the jest tests, are you? They run and pass, but typescript is barking.
Multiple PRs would have been nice, agree with @katconnors. Maybe a Jest PR, Storybook PR, and then the final Breadcrumb PR.

Still looking at and running the code, but those are some first thoughts.

Lastly, and I know a redesign wasn't part of this, but to springboard some discussions - we really need more ui/ux design on the breadcrumb trail to know exactly where the app should go. When I've drilled into a goal:

I really expect 'Jane Student' to be clickable.
I don't really understand the difference between what would happen clicking on 'Jane Student' vs. clicking on 'GOALS'.
'Students' makes sense.
Don't ever click on 'GOALS' .. wow, app went into total crash mode, nothing works, getting weird hydration errors. Even hand-entering /students in the address bar no longer works. Love next.js!!!
uuids in the breadcrumb are not helpful.

src/components/design_system/breadcrumbs/transformBreadcrumbs.ts

src/components/design_system/breadcrumbs/useBreadcrumbContext.ts

mrabbitt · 2024-09-23T23:12:14Z

Lastly, and I know a redesign wasn't part of this, but to springboard some discussions - we really need more ui/ux design on the breadcrumb trail to know exactly where the app should go. When I've drilled into a goal:
1. I really expect 'Jane Student' to be clickable.

2. I don't really understand the difference between what would happen clicking on 'Jane Student' vs. clicking on 'GOALS'.

3. 'Students' makes sense.

4. Don't ever click on 'GOALS' .. wow, app went into total crash mode, nothing works, getting weird hydration errors. Even hand-entering /students in the address bar no longer works. Love next.js!!!

5. uuids in the breadcrumb are not helpful.

@thomhickey I had filed #396 to address most of these issues. My plan was to first add the ability to test the logic within the breadcrumbs component without changing its functional behavior in this PR, and then fix the functional behavior in another PR and close out #396.

mrabbitt · 2024-09-23T23:56:57Z

5. I'm getting a lot of 'unsafe call of an any typed value' in the jest tests, are you? They run and pass, but typescript is barking.

@thomhickey Hmm...I'm not seeing that. Where do you see that message?

KCCPMG · 2024-09-24T21:19:24Z

@mrabbitt

Awesome stuff here. Storybook is blowing my mind, what a huge time saver. Can't count on a million fingers how many times I've drilled into a UI just to see my latest changes of some internal component. HMR alleviates some of this, but storybook is a whole new level.

Let's go with Jest everywhere as far as I'm concerned. I'd be happy to help rewrite the ava tests into Jest. It's a shame to have two test runners. If not, then Jest front-end, Ava back-end is fine with me. The way you've integrated them both into package.json is great.

For names I suggest BreadcrumbTrail (parent) and Breadcrumb (child) for the two components - note singular not plural.

Love the snapshot testing.

I'm getting a lot of 'unsafe call of an any typed value' in the jest tests, are you? They run and pass, but typescript is barking.

Multiple PRs would have been nice, agree with @katconnors. Maybe a Jest PR, Storybook PR, and then the final Breadcrumb PR.

Still looking at and running the code, but those are some first thoughts.

Lastly, and I know a redesign wasn't part of this, but to springboard some discussions - we really need more ui/ux design on the breadcrumb trail to know exactly where the app should go. When I've drilled into a goal:
1. I really expect 'Jane Student' to be clickable. 2. I don't really understand the difference between what would happen clicking on 'Jane Student' vs. clicking on 'GOALS'. 3. 'Students' makes sense. 4. Don't ever click on 'GOALS' .. wow, app went into total crash mode, nothing works, getting weird hydration errors. Even hand-entering /students in the address bar no longer works. Love next.js!!! 5. uuids in the breadcrumb are not helpful.

I have no familiarity with storybook, but when I run it, all the stories are broken. I'm guessing that's not what's supposed to happen...is there anything I should be doing or checking?

EDIT: Nvm, for some reason the "transformBreadcrumbs" file was named "transformBreadCrumbs" and that caused storybook to break. Not sure how that happened.

…eview.tsx

mrabbitt · 2024-09-24T23:08:31Z

For names I suggest BreadcrumbTrail (parent) and Breadcrumb (child) for the two components - note singular not plural.

@thomhickey I think that would make sense if the parent displayed the trail of breadcrumbs and the child displayed a single "crumb," but that's not what's happening. The parent is a stateful component which gets path information from the next router and name data from the backend, and passes it to the child component as props. The stateless child component displays the entire breadcrumb trail based on the props. I like the idea of using "BreadcrumbTrail" rather than "Breadcrumbs" plural though.

When @nickvisut and I discussed the PR over a huddle last week, he suggested that:

The inner, stateless component should be called something minimal like "Breadcrumbs" (I think "BreadcrumbTrail" could work too), and should stay in the design_system directory.
The outer, stateful component's name should reflect that it's a stateful wrapper that doesn't directly handle display, such as "BreadcrumbsContainer" or "StatefulBreadcrumbs" or something similar. Because of its role, it should not be under the design_system directory.

(@nickvisut , please correct me if I'm misrepresenting your suggestions.)

What I'm planning to do now:

Follow @nickvisut 's suggestions for structure.
Use "BreadcrumbTrail" as the stateless inner component and "StatefulBreadcrumbTrail" as the stateful wrapper.
Rename the transformBreadcrumbs.ts file to BreadcrumbTrailUtils.ts (following Thom's suggestion here)

I'll work on this now. Thanks for the feedback, @thomhickey and @nickvisut !

mrabbitt · 2024-09-24T23:14:21Z

EDIT: Nvm, for some reason the "transformBreadcrumbs" file was named "transformBreadCrumbs" and that caused storybook to break. Not sure how that happened.

@KCCPMG, sorry about that. This might have happened because I accidentally named that file with a capital "C"in an earlier commit, and then renamed it with a lower case "c" to match the other files in a later commit. When this PR is eventually squashed and merged, that renaming should be removed from git history, so hopefully no one else will run into this. 🤞 Glad it's working for you now.

KCCPMG · 2024-09-24T23:18:00Z

EDIT: Nvm, for some reason the "transformBreadcrumbs" file was named "transformBreadCrumbs" and that caused storybook to break. Not sure how that happened.

@KCCPMG, sorry about that. This might have happened because I accidentally named that file with a capital "C"in an earlier commit, and then renamed it with a lower case "c" to match the other files in a later commit. When this PR is eventually squashed and merged, that renaming should be removed from git history, so hopefully no one else will run into this. 🤞 Glad it's working for you now.

No worries, I felt embarrassed by what a relatively minor issue it was, and I'm surprised it ended up out of sync somehow when it wasn't caught when I started up the the dev server. At any rate, I would have deleted, but I'm leaving it up in case anyone else runs into the same problem and can be spared my embarrassment.

package-lock.json taken from main branch, then ran `npm install`. # Conflicts: # package-lock.json

thomhickey · 2024-09-25T00:30:43Z

For names I suggest BreadcrumbTrail (parent) and Breadcrumb (child) for the two components - note singular not plural.

@thomhickey I think that would make sense if the parent displayed the trail of breadcrumbs and the child displayed a single "crumb," but that's not what's happening. The parent is a stateful component which gets path information from the next router and name data from the backend, and passes it to the child component as props. The stateless child component displays the entire breadcrumb trail based on the props. I like the idea of using "BreadcrumbTrail" rather than "Breadcrumbs" plural though.

When @nickvisut and I discussed the PR over a huddle last week, he suggested that:

The inner, stateless component should be called something minimal like "Breadcrumbs" (I think "BreadcrumbTrail" could work too), and should stay in the design_system directory.

The outer, stateful component's name should reflect that it's a stateful wrapper that doesn't directly handle display, such as "BreadcrumbsContainer" or "StatefulBreadcrumbs" or something similar. Because of its role, it should not be under the design_system directory.

(@nickvisut , please correct me if I'm misrepresenting your suggestions.)

What I'm planning to do now:

Follow @nickvisut 's suggestions for structure.

Use "BreadcrumbTrail" as the stateless inner component and "StatefulBreadcrumbTrail" as the stateful wrapper.

Rename the transformBreadcrumbs.ts file to BreadcrumbTrailUtils.ts (following Thom's suggestion here)

I'll work on this now. Thanks for the feedback, @thomhickey and @nickvisut !

Thanks @mrabbitt ... yeah, I suppose I need to be brought up to speed on this whole 'design' and 'design_system' paradigm that's in the codebase. Still scratching my head about why the word 'design' is coming up anywhere in the actual implementations of these components. I understand terms like 'layout' or 'container' in page and component filenames, but 'design' is a head-scratcher for me.

thomhickey · 2024-09-25T00:37:06Z

I'm getting a lot of 'unsafe call of an any typed value' in the jest tests, are you? They run and pass, but typescript is barking.

@thomhickey Hmm...I'm not seeing that. Where do you see that message?

it went away. that's the second or third time my cursor/vscode has barked about typescript errors that ended up going away. not happy! ... sorry for the confusion @mrabbitt

KCCPMG · 2024-09-25T01:23:00Z

I'm getting weird 'cannot find module' errors, even after re-running npm install and even removing my node_modules first and then doing it again. Has anyone else had any issues with this? The first one I had was with esbuild-runner, which I decided to install just to make things work. I got past the postinstall point where it was usually failing, but my backend tests are all exiting and telling me that it cannot find 'nextjs-middleware-wrappers'. This has been the case when I run both 'npm run test:backend', but also if I just do 'npx ava' (though that also gives me grief about finding jest files).

mrabbitt · 2024-09-26T00:50:23Z

but also if I just do 'npx ava' (though that also gives me grief about finding jest files).

@KCCPMG , I know this isn't the main problem, but I just pushed a small config change that makes npx ava run only the backend tests so it won't complain about jest files anymore. 🙂

mrabbitt · 2024-09-26T00:58:31Z

I'm getting weird 'cannot find module' errors, even after re-running npm install and even removing my node_modules first and then doing it again. Has anyone else had any issues with this? The first one I had was with esbuild-runner, which I decided to install just to make things work. I got past the postinstall point where it was usually failing, but my backend tests are all exiting and telling me that it cannot find 'nextjs-middleware-wrappers'. This has been the case when I run both 'npm run test:backend', but also if I just do 'npx ava' (though that also gives me grief about finding jest files).

@KCCPMG Weird. I haven't seen this. I tried wiping node_modules and re-running npm install and npn run all:check, and everything looked good.

Are there files installed for you under node_modules/nextjs-middleware-wrappers/ ? I see:

$ ls node_modules/nextjs-middleware-wrappers
README.md          dist/              release.config.js  tsconfig.json
ava.config.js      package.json       tests/             wrappers.ts

One other idea that might be worth trying is doing a fresh git checkout of compass in another location and seeing if the problem happens there too.

KCCPMG · 2024-09-28T01:55:10Z

I'm getting weird 'cannot find module' errors, even after re-running npm install and even removing my node_modules first and then doing it again. Has anyone else had any issues with this? The first one I had was with esbuild-runner, which I decided to install just to make things work. I got past the postinstall point where it was usually failing, but my backend tests are all exiting and telling me that it cannot find 'nextjs-middleware-wrappers'. This has been the case when I run both 'npm run test:backend', but also if I just do 'npx ava' (though that also gives me grief about finding jest files).

@KCCPMG Weird. I haven't seen this. I tried wiping node_modules and re-running npm install and npn run all:check, and everything looked good.

Are there files installed for you under node_modules/nextjs-middleware-wrappers/ ? I see:
$ ls node_modules/nextjs-middleware-wrappers
README.md          dist/              release.config.js  tsconfig.json
ava.config.js      package.json       tests/             wrappers.ts
One other idea that might be worth trying is doing a fresh git checkout of compass in another location and seeing if the problem happens there too.

I just tried the updated branch, then checked out main, all the same thing. I also tried looking for node_modules/nextjs-middleware-wrappers, and the directory isn't there. It looks like it's not supposed to be in package.json, but I do see it in the package-lock.json several times as a peer dependency. The other weird thing is that running tests (or doing a type check) says that now there's no declaration file for the 'pg' library. I'm not sure what's going on, it seems like I might have something pretty deeply screwed up.

UPDATE:

I tried running this on my other machine and it seems to work fine. More tellingly, running npm install doesn't change my package-lock.json to be out of sync with the git repository, which was happening with both my main branch as well as this one. Seems like the issue is isolated to my MacBook, hopefully the exorcist I called can tell me more.

KCCPMG

I'll approve this because it seems to work, and because the jest testing and storybook stuff look terrific. As far as I'm concerned, this can be merged, but below are my quibbles, which you can feel free to disregard or respond to:

I mentioned this before, but it seems to me that useBreadcrumbContext should not be called what it is. There's no call to useContext, nor is there a BreadcrumbContextProvider or something similar, it's effectively just a formatted data object to be passed down. Along similar lines, I don't think that the useBreadcrumbContext needs its own file, but that's an even more nitpicky convention thing, about which I'm probably wrong.
I don't think I quite understand why the parent component is referred to as Stateful, there's no actual state and it looks like it's just hooking into useRouter and then passing re-formatted data down.
Honestly I get a little bit confused jumping back and forth through the key 4 files involved here, but that's much more on me. I can see how you're splitting up the path and making the "descriptor" part of the benchmark linkable, but not the value that corresponds to a specific identifier. I suppose that makes sense, but it does mean that in students/student_id/goals/goal_id, you wouldn't be able to return to the student, but you would have a link to a goals page that isn't there (I believe @thomhickey mentioned that particular one as well).

Like I said, this works, and hopefully there's some meaningful feedback on the design side of things in terms of if there's something different that they'd like to see. I probably would have used an object that looks at the descriptor which then determined how the descriptor and identifier would be rendered, but that's off the top of my head and there might be very good reasons for not doing that. Sorry for taking so long to get back to you on the review, good job with this, this was clearly a ton of work, and I think that the other infrastructure aspects of this PR will pay big dividends long-term.

KCCPMG · 2024-09-24T20:13:12Z

src/components/design_system/breadcrumbs/__snapshots__/Breadcrumbs.test.tsx.snap

@@ -0,0 +1,227 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`BreadcrumbsDesign renders empty for / 1`] = `


I just found this comment by doing a search for "readme"... I would definitely advocate for updating the README. My personal best guess would be to go into the h3 "Running Tests", put all the existing content into an h4 for "Backend Tests", add information for "Frontend Tests" as another h4 below it, and then lastly one more h4 for "All Tests". It might be good to also add a link to https://jestjs.io/docs/snapshot-testing, which is where I just went to try to remember how snapshot testing works.

Also, the number seems to refer to the number of the test. Just playing around with it now, I created another test "renders empty for /students/student-id" with different props, and the resulting test generated an additional snapshot, so now there's a

exports[BreadcrumbsDesign renders empty for /students/student-id 2] = `

snapshot in addition to the first one on my local branch version of the snapshot file.

src/components/design_system/breadcrumbs/useBreadcrumbContext.ts

KCCPMG · 2024-09-25T00:30:39Z

src/components/breadcrumbs/StatefulBreadcrumbTrailContext.ts

+
+export type Student = SelectableForTable<"student">;
+export type Para = SelectableForTable<"user">;
+export interface BreadcrumbContext {


I'm late to the naming things party, but I would caution against using the word context in both this interface and the function. There's no provider for this context as a parent component that deeply nested child components are using, so to my mind this doesn't behave the way that I would expect of something with this name.

following up on this, I agree that we don't want to overload the concept of React "context". perhaps "data" or something similar could work, @mrabbitt?

katconnors

No change to-dos from my end, for your commits since my last comment ( Sep 24th onward).

nickvisut · 2024-10-19T02:47:10Z

src/components/breadcrumbs/StatefulBreadcrumbTrailContext.ts

+
+export type Student = SelectableForTable<"student">;
+export type Para = SelectableForTable<"user">;
+export interface BreadcrumbContext {


following up on this, I agree that we don't want to overload the concept of React "context". perhaps "data" or something similar could work, @mrabbitt?

nickvisut · 2024-10-19T02:48:38Z

src/components/breadcrumbs/StatefulBreadcrumbTrailContext.ts

+  person: Student | Para | undefined;
+}
+
+export const useBreadcrumbContext = (


Is this a custom hook?

(if not, can we replace the use with another word?)

nickvisut · 2024-10-19T02:55:57Z

Revisiting this and it's good stuff, @mrabbitt. Looks like there are a handful of unresolved convos (and I just added a couple of my own). They could either be moved out w/"Reference in new issue" to address later or resolved in this PR.

mrabbitt added 24 commits August 27, 2024 15:19

WIP

80e6ee7

WIP

1c98917

Refactored logic and state out of BreadcrumbNav.

45af0fe

WIP: Jest test for Breadcrumb, with mock of useRouter() working!

6e7915a

trpc dependency not handled yet

Fixed imports

299fa6a

WIP: Mocking for trpc not working yet

7cb160a

Use a class instead of inline styling for non-link breadcrumbs.

859e062

Refactored to use router.query, move path splitting out of Breadcrumb…

e9c8f6f

…sNav.

Test transformBreadCrumbs() using Jest snapshots.

4399482

Fixed missing newline causing prettier check to fail.

99936bf

Refactored rendering out of transform function.

e393e8a

Renamed file to .ts and fixed type linting error.

eda5c9e

Updated transformer tests to test objects instead of snapshots.

8ff2bf7

Fixed test and uppercase behavior.

61b58f2

Removed now unncessary jest-dom import from test

cf2a1be

Merge branch 'unit-test/storybook' into unit-test/tsx-jest

4bccad1

(Reverted changes to package.json for storybook) # Conflicts: # package-lock.json # package.json # src/components/design_system/breadcrumbs/Breadcrumbs.tsx

Refactored data access into wrapper component, stories now working fo…

558317a

…r inner display-only component.

Ignore storybook-static.

6f5179d

Ignore storybook-static for eslint and prettier.

4c92567

Updated jest tests to use snapshots for testing inner presentational …

5ab0320

…component

Fixed import in NavBar and eslint config formatting.

85b87fd

Removed intermediary BreadcrumbDesign component.

41f4b27

Cleaning up names, added BreadcrumbContext to hold more than just per…

98558b0

…son data.

Fixed case-sensitive renaming

5655840

mrabbitt mentioned this pull request Sep 17, 2024

Breadcrumbs refactor, testing using ava & storybook #414

Draft

mrabbitt requested review from nickvisut, thomhickey, katconnors, BeeSeeWhy and KCCPMG September 17, 2024 21:19

katconnors reviewed Sep 17, 2024

View reviewed changes

mrabbitt commented Sep 20, 2024

View reviewed changes

mrabbitt mentioned this pull request Sep 20, 2024

Fix Storybook argTypesRegex warning #421

Open

mrabbitt commented Sep 20, 2024

View reviewed changes

package.json Outdated

"lint-staged": "^13.1.0",

"next-router-mock": "^0.9.13",

Copy link

Contributor Author

mrabbitt Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Remove this

thomhickey reviewed Sep 20, 2024

View reviewed changes

PR TODOs: Removed unused next-router-mock, revert argTypesRegex in pr…

bf9bccc

…eview.tsx

mrabbitt added 2 commits September 24, 2024 16:52

Renamed components to BreadcrumbTrail, StatefulBreadcrumbTrail

555e50b

Merge branch 'main' into breadcrumb-refactor/jest

eb746b9

package-lock.json taken from main branch, then ran `npm install`. # Conflicts: # package-lock.json

mrabbitt marked this pull request as ready for review September 25, 2024 00:11

Update ava.config.js so that npx ava only runs backend tests.

1ca1c1c

KCCPMG approved these changes Oct 15, 2024

View reviewed changes

katconnors self-requested a review October 16, 2024 18:41

katconnors reviewed Oct 16, 2024

View reviewed changes

nickvisut reviewed Oct 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breadcrumbs refactor, testing using jest & storybook #413

Breadcrumbs refactor, testing using jest & storybook #413

mrabbitt commented Sep 17, 2024 •

edited

Loading

katconnors Sep 17, 2024 •

edited

Loading

mrabbitt Sep 23, 2024

mrabbitt Sep 23, 2024

KCCPMG Sep 24, 2024

katconnors left a comment

mrabbitt Sep 20, 2024

mrabbitt Sep 20, 2024

mrabbitt Sep 20, 2024

mrabbitt Sep 20, 2024

thomhickey commented Sep 20, 2024

mrabbitt commented Sep 23, 2024 •

edited

Loading

mrabbitt commented Sep 23, 2024

KCCPMG commented Sep 24, 2024 •

edited

Loading

mrabbitt commented Sep 24, 2024

mrabbitt commented Sep 24, 2024

KCCPMG commented Sep 24, 2024

thomhickey commented Sep 25, 2024

thomhickey commented Sep 25, 2024

KCCPMG commented Sep 25, 2024

mrabbitt commented Sep 26, 2024 •

edited

Loading

mrabbitt commented Sep 26, 2024

KCCPMG commented Sep 28, 2024 •

edited

Loading

KCCPMG left a comment

KCCPMG Sep 24, 2024

KCCPMG Sep 25, 2024

nickvisut Oct 19, 2024

katconnors left a comment

nickvisut Oct 19, 2024

nickvisut Oct 19, 2024

nickvisut Oct 19, 2024

nickvisut commented Oct 19, 2024 •

edited

Loading

		@@ -0,0 +1,227 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		exports[`BreadcrumbsDesign renders empty for / 1`] = `

		// eslint-disable-next-line @typescript-eslint/no-unsafe-return
		jest.mock("next/router", () => require("next-router-mock"));

Breadcrumbs refactor, testing using jest & storybook #413

Are you sure you want to change the base?

Breadcrumbs refactor, testing using jest & storybook #413

Conversation

mrabbitt commented Sep 17, 2024 • edited Loading

Included in this PR

Not Included in this PR

What input is especially requested from reviewers:

katconnors Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katconnors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomhickey commented Sep 20, 2024

mrabbitt commented Sep 23, 2024 • edited Loading

mrabbitt commented Sep 23, 2024

KCCPMG commented Sep 24, 2024 • edited Loading

mrabbitt commented Sep 24, 2024

mrabbitt commented Sep 24, 2024

KCCPMG commented Sep 24, 2024

thomhickey commented Sep 25, 2024

thomhickey commented Sep 25, 2024

KCCPMG commented Sep 25, 2024

mrabbitt commented Sep 26, 2024 • edited Loading

mrabbitt commented Sep 26, 2024

KCCPMG commented Sep 28, 2024 • edited Loading

KCCPMG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katconnors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvisut commented Oct 19, 2024 • edited Loading

mrabbitt commented Sep 17, 2024 •

edited

Loading

katconnors Sep 17, 2024 •

edited

Loading

mrabbitt commented Sep 23, 2024 •

edited

Loading

KCCPMG commented Sep 24, 2024 •

edited

Loading

mrabbitt commented Sep 26, 2024 •

edited

Loading

KCCPMG commented Sep 28, 2024 •

edited

Loading

nickvisut commented Oct 19, 2024 •

edited

Loading