Skip to content

Conversation

@sandrahoang686
Copy link
Contributor

@sandrahoang686 sandrahoang686 commented Dec 11, 2025

Hey @gadomski, I figured it would be quicker if I just made the changes I was going to comment onto your PR to hopefully be quicker. Let me know what you think? Should be able to follow my commits. I made sure to validate the behavior matched prod and these changes did - feel free to manually validate as well!

Changes:

  • Updated the useDocumentTitle to no longer be a hook and instead made it a fn. The useEffect which depended on value from useStacValue - I just kept in app.tsx because thats the only place that would need that subscription so I didn't think a separate hook was needed.
  • Cleaned up useHrefParam
  • Updated useStacFilters to either return the filtered assets or the original assets array instead of undefined. This way we dont have to pass in collections & filteredCollections separately (same with items).
    • So => collections={filteredCollections} and same with items

@sandrahoang686 sandrahoang686 changed the title Updates to isolated hooks refactor: Updates to isolated hooks Dec 11, 2025
Copy link
Collaborator

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

In general, makes sense, thanks for the fixups! One filename nit, and one bug. Don't know quite where it broke, but the collection filtering makes the filter disappear under this case:

Screen.Recording.2025-12-11.at.5.17.58.PM.mov

The same might be an issue for items as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't love utils/utilties.ts as a file name. I think a top-level utils.tsx should be good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to utils/index.ts because didn't know if we wanted a utils/ dir and then a utils.tsx file at the same level

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe utils/title.ts?

@sandrahoang686
Copy link
Contributor Author

@gadomski thanks for reviewing! I couldn't replicate the bug though...

Screen.Recording.2025-12-14.at.7.02.12.PM.mov

is how its behaving for me locally. wasn't able to get it to dissapear and tried other things - you have other replication steps?

@gadomski
Copy link
Collaborator

Reproduction steps:

  1. Go to http://localhost:5173/stac-map/?href=https://stac.eoapi.dev/
  2. Click "Fetch all collections"
  3. Click on the "Filter" pane
  4. Drag the right side of the slider to the left
Screen.Recording.2025-12-15.at.6.14.01.AM.mov

Copy link
Collaborator

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

In general, makes sense, thanks for the fixups! One filename nit, and one bug. Don't know quite where it broke, but the collection filtering makes the filter disappear under this case:

Screen.Recording.2025-12-11.at.5.17.58.PM.mov

The same might be an issue for items as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe utils/title.ts?

@sandrahoang686
Copy link
Contributor Author

sandrahoang686 commented Dec 16, 2025

@gadomski alright solved for bug - what happened was the Filter component relied on the collections & items assets instead of the filtered. Because my changes only now pass the filtered assets - datetimes couldn't be calculated in the component with the pre-filtered assets.

So I isolated that functionality into a fn getDateTimes and calculate it at the top level since that is where those assets are and pass it in as a prop. Not great for prop drilling but I think better than originally passing in the pre-filtered assets instead!

This is ready for re-review 🙇🏼

Copy link
Collaborator

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for this!

Copy link
Collaborator

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for this!

@gadomski gadomski merged commit a5f1d74 into refactor Dec 16, 2025
6 checks passed
@gadomski gadomski deleted the refactor-changes branch December 16, 2025 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants