DR-3549: Emma repo offboarding by 7emansell · Pull Request #338 · NYPL/digital-collections

7emansell · 2025-05-28T20:16:35Z

Ticket:

JIRA ticket DR-3549

This PR does the following:

Repo offboarding and clean up:

Reorganizes code into folders where possible, renames types more strictly
Deletes unused hooks and mocks
Makes changelog update from Husky pre-commit on qa only
Minor adjustments to README

Open Questions

How has this been tested? How should a reviewer test this?

Accessibility concerns or updates

Checklist:

I have added relevant accessibility documentation for this pull request.
All new and existing tests passed.
I have updated the CHANGELOG.md.

vercel · 2025-05-28T20:16:40Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
digital-collections	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 4, 2025 8:27pm

EdwinGuzman

Before I approve, can you link to the ticket for the collectionStructure refactor/update? It's not significant but want to make be clear on the reqs for the forced loading state.

A few notes:

For a page such as https://digital-collections-git-dr-3549-repo-offboarding-nypl.vercel.app/collections/2589a880-c52c-012f-2cb4-58d385a7bc34, it's confusing to see just the letter applied as a filter without more context... I'm sure it's clear because the user is intentionally selecting that filter, but otherwise, it's confusing if you're looking at it randomly:

The dimmed style state works well but it's not a "true" loading state where the buttons are disabled. I can still click around while the results update and get the next set of results. I guess it's scope creep but the buttons should be disabled to not queue up fetch requests.
I thought this was a problem with this PR but I can replicate it on prod: clicking around sibling filters, I can toggle one, select another, and try to toggle back but then either 1) I land on an indefinite loading state with no search results (and no error message in the console), or 2) the previously clicked filter is toggled and then it seems to jump around. It will be better to show you in person.

EdwinGuzman · 2025-06-02T21:17:51Z

.husky/post-merge

Just adding a comment as a reminder to review this once it's merged as we discussed offline.

EdwinGuzman · 2025-06-02T21:27:56Z

app/collections/[uuid]/page.tsx

+import { CollectionsApi } from "@/src/utils/apiClients/apiClients";
+import { AvailableFilterOption } from "@/src/types/AvailableFilterType";
+
+export type CollectionSearchParamsType = {


This isn't wrong but moving the type to this file now makes this file a dependency for /app/collections/page.tsx, and this also means importing AvailableFilterOption just for this type. Again, not wrong but it's a code smell and this tends to end up creating circular dependencies.

EdwinGuzman · 2025-06-02T21:50:23Z

app/src/components/collectionStructure/accordionTree.tsx

  items,
  toggle,
  targetUuid,
+  toggledUuid,


Add documentation for this prop to the comment above. It's not clear that this is used for the dimmed style.

EdwinGuzman · 2025-06-02T21:51:43Z

app/src/components/collectionStructure/accordionTree.tsx

+  toggledUuid: string;
 }) => {
+  const ancestorUuids = targetUuid
+    ? findAncestorUuids(items, targetUuid) ?? []


Since findAncestorUuids is only used in this file, why not make it return an empty array instead of null so you don't have to ?? [] here?

EdwinGuzman · 2025-06-02T21:55:32Z

app/src/components/collectionStructure/accordionTree.tsx

  );
 };

+const findAncestorUuids = (


I don't think you need comment documentation for all functions, but this recursive function is a good candidate for further documentation. At first glance, it's complicated enough that makes me think it's related to how the component functions and renders, but it's only used for styling properly.

EdwinGuzman · 2025-06-02T21:56:59Z

app/src/components/collectionStructure/collectionStructure.tsx

  searchManager.setLastFilter(null);

-  /* 
-  If opening the item, add the subcollection filter. If closing, remove 


Is this no longer relevant?

EdwinGuzman · 2025-06-02T21:57:19Z

app/src/components/collectionStructure/collectionStructure.tsx

+    }
  };

-  // If no subcontainers, don't display collection structure


No longer relevant?

EdwinGuzman · 2025-06-02T22:05:34Z

app/src/components/collectionStructure/collectionStructure.tsx

+      console.error("Toggle failed:", error);
+    } finally {
+      setTimeout(() => {
+        setToggledUuid(null);


By doing this, aren't you rerendering the entire AccordionTree with this state update? It seems unnecessary for the style update.

EdwinGuzman · 2025-06-02T22:06:10Z

app/src/components/collectionStructure/collectionStructure.tsx

  isOpen: boolean;
  hasSubContainers: boolean;
  children?: OpenStateItem[];
+  parentUuid?: string | null;


I'm having a hard time following this but why and how is parentUuid used? I see it being set but not read.

Used to fetch siblings of the current node in buildTreeWithPathToUuid

EdwinGuzman · 2025-06-02T22:16:10Z

Besides the comments above, the actual offboarding updates in this PR look fine, thanks.

7emansell · 2025-06-04T14:01:06Z

Before I approve, can you link to the ticket for the collectionStructure refactor/update? It's not significant but want to make be clear on the reqs for the forced loading state.

A few notes:

For a page such as https://digital-collections-git-dr-3549-repo-offboarding-nypl.vercel.app/collections/2589a880-c52c-012f-2cb4-58d385a7bc34, it's confusing to see just the letter applied as a filter without more context... I'm sure it's clear because the user is intentionally selecting that filter, but otherwise, it's confusing if you're looking at it randomly:

* The dimmed style state works well but it's not a "true" loading state where the buttons are disabled. I can still click around while the results update and get the next set of results. I guess it's scope creep but the buttons should be disabled to not queue up fetch requests. * I _thought_ this was a problem with this PR but I can replicate it on prod: clicking around sibling filters, I can toggle one, select another, and try to toggle back but then either 1) I land on an indefinite loading state with no search results (and no error message in the console), or 2) the previously clicked filter is toggled and then it seems to jump around. It will be better to show you in person.

As discussed in person, no ticket/reqs for this loading state, this work likely needs to be more clearly defined before it can be completed.

Some subcollections do have obscure titles like "Vol II", not sure what we can do besides always include the collection title as well ex. "Posada Collection: Vol II". Something to check with Lana on
Dimmed state not a true loading state but mainly meant to prevent user from doing what you describe above– clicking around so quickly that the collection structure gets stuck or seems to be jumping around as it catches up
Tldr this should be addressed separately in the work here: Collection structure refinement PR

7emansell added 5 commits May 21, 2025 11:15

Loading state kind of for collection structure

b48e415

Merge branch 'main' into no_ref/collection-landing-improvements

5798b79

Updating blur behavior to unblur parent uuids

9bb37ef

PR process updates

1cdefe2

Reorganizing and deleting

311ee6e

vercel bot had a problem deploying to Preview May 28, 2025 20:19 Failure

Correcting types

ab926e5

vercel bot deployed to Preview May 28, 2025 20:25 View deployment

Removing collection inclusion handling

deede0e

vercel bot deployed to Preview May 29, 2025 20:11 View deployment

More typing and mocks

b2ce1c3

vercel bot deployed to Preview June 2, 2025 13:46 View deployment

readme adjustments

ff1cd32

vercel bot deployed to Preview June 2, 2025 13:52 View deployment

7emansell marked this pull request as ready for review June 2, 2025 13:52

7emansell requested review from EdwinGuzman and avertrees June 2, 2025 13:52

containsOnSiteMaterials should be a boolean

c293e3a

vercel bot deployed to Preview June 2, 2025 14:03 View deployment

EdwinGuzman reviewed Jun 2, 2025

View reviewed changes

7emansell added 2 commits June 4, 2025 09:51

Making it pre-commit bc i'm doubting myself

6cadcd0

Removing collection structure stuff

2402315

vercel bot had a problem deploying to Preview June 4, 2025 13:57 Failure

Merge branch 'main' into DR-3549/repo-offboarding

e1f6c1c

vercel bot deployed to Preview June 4, 2025 20:27 View deployment

7emansell merged commit 936a069 into main Jun 6, 2025
4 checks passed

7emansell deleted the DR-3549/repo-offboarding branch June 6, 2025 19:32

Conversation

7emansell commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket:

This PR does the following:

Open Questions

How has this been tested? How should a reviewer test this?

Accessibility concerns or updates

Checklist:

Uh oh!

vercel bot commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EdwinGuzman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdwinGuzman commented Jun 2, 2025

Uh oh!

7emansell commented Jun 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

7emansell commented May 28, 2025 •

edited

Loading

vercel bot commented May 28, 2025 •

edited

Loading

EdwinGuzman left a comment •

edited

Loading