Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Populate the global tissue filter with the tissue in the tissue card #5673

Closed
5 tasks
prathapsridharan opened this issue Aug 31, 2023 · 16 comments
Closed
5 tasks
Assignees
Labels
data-viz Data Viz Team P0 Priority 0 - Critical, fix ASAP!

Comments

@prathapsridharan
Copy link
Contributor

prathapsridharan commented Aug 31, 2023

TODO list:

  • Convert tissueId from the URL from _ to :
  • Update selecting organ state to trigger route change to new URL, so URL becomes the tissueId source of truth
  • Make sure /cellguide/[:cellTypeId] still works
  • Write tests
  • IMPORTANT: Need to work with Analytics, since Amanda says likely there will be big changes involved on the analytics side
@prathapsridharan
Copy link
Contributor Author

When a user goes from a tissue card to a cell card, the cell card page should have the tissue from which the user navigated from automatically pre-selected in the global tissue dropdown

@prathapsridharan prathapsridharan added data-viz Data Viz Team P0 Priority 0 - Critical, fix ASAP! labels Aug 31, 2023
@joyceyan
Copy link
Contributor

joyceyan commented Sep 7, 2023

General approach:

  • Once we're in the CellGuideCardSearchBar, we need to know what the old entity id we're currently on is, as well as the new entity id we're navigating to
  • I think it makes most sense to use the useContext react hook. I actually haven't worked with this too much, but this seems like the best option to me. It basically manages a global state, so you can store both the current entity ID as well as the last entity ID in this global state.
  • An alternative approach would be to pass the current entity id down all the way to CellGuideCardSearchBar through props drilling, and probably pass the old entity id through a URL query param. I think this will work but it seems a bit more complicated.

Since #5674 needs to be completed, I think including this as URL query params makes the most sense.

@tihuan
Copy link
Contributor

tihuan commented Oct 18, 2023

I think this ticket is not ready for development, since engineers still need to decide on an approach. So we can't point this ticket yet!

Yeah I think we can expand #5674 to include a new URL path for tissue specific cell type page like so:

  1. Tissue - cellguide/tissues/:id (we're doing this already)
  2. Tissue specific cell type - cellguide/tissues/:id/cell-types/:cellTypeId (this will be new)
  3. Tissue agnostic cell type - cellguide/:id (we're doing this already)

This way the link is sharable and state is encoded in the URL, and it will resolve this ticket as well

We can also consider expanding SEO sitemap to include the new paths from #2 as part of the ticket!

CC: @dsadgat @niknak33 @joyceyan @prathapsridharan

Thanks all!

@tihuan
Copy link
Contributor

tihuan commented Oct 23, 2023

CC: @ainfeld

@ainfeld
Copy link
Contributor

ainfeld commented Oct 23, 2023

Thanks @tihuan! Please keep me updated on designs and urls as analytics will need to be changed quite a bit to encompass these changes. Thanks 🙏

@tihuan
Copy link
Contributor

tihuan commented Nov 13, 2023

Ah @ainfeld mentioned above that analytics might need to be changed quite a bit to adapt to the URL change, since she's gonna be out for a while, who's helping out with analytics in the meantime?

CC: @niknak33 @dsadgat

Thank you!

@tihuan
Copy link
Contributor

tihuan commented Nov 13, 2023

Confirmed that Next.js can handle new URL with frontend/src/pages/cellguide/tissues/[tissueId]/cell-types/[cellTypeId].tsx

e.g., https://localhost:3000/cellguide/tissues/UBERON_0002048/cell-types/CL_0001061

@tihuan
Copy link
Contributor

tihuan commented Nov 13, 2023

I estimated 8 points to take into the unknown effort involved in updating analytics

The feature related change + tests should be around 5 points

@tihuan
Copy link
Contributor

tihuan commented Nov 13, 2023

I'll start working on this ticket tomorrow!

@tihuan
Copy link
Contributor

tihuan commented Nov 21, 2023

The feature part is ready for code review (CC: @atarashansky 🙏), but keeping it in WIP column, so we're aware that we're still missing the analytics piece

@ainfeld
Copy link
Contributor

ainfeld commented Nov 27, 2023

Thank you so much @tihuan! Is there an rdev link for me to play around with the planned updates and figure out what analytics may need to be updated? 🙏

@ainfeld
Copy link
Contributor

ainfeld commented Nov 27, 2023

From reviewing your comment on October 18, my hunch is no engineering updates will be required for analytics. I'll just need to update how I'm calculating a bunch of current cell guide measurements. This advanced warning is much appreciated!!

@tihuan
Copy link
Contributor

tihuan commented Nov 30, 2023

Hey @ainfeld welcome back! Sorry for the slow reply due to upcoming bereavement 🙏

Yes! Rdev links are automatically posted in the PR. (Something like this!)

Here's a tissue specific cell type page, for example:
https://pr-6232-frontend.rdev.single-cell.czi.technology/cellguide/tissues/UBERON_0002369/cell-types/CL_0000540

Things to look out for is that since the tissue specific cell type page now has a new URL structure, if you were relying on just /cellguide/:cellTypeId to capture all cell type page visits, we'll now need both /cellguide/:cellTypeId and cellguide/tissues/:tissueId/cell-types/:cellTypeId to capture all cell type pages' visits

The Plausible tracking event that we fire when we change tissue is still happening, so hopefully nothing needs to change there. But happy to make any eng adjustments as needed this week (but I'll be off for 2 weeks starting Monday, so someone else might need to take over CC: @dsadgat )

Just let me know! Thank you!

@tihuan
Copy link
Contributor

tihuan commented Nov 30, 2023

@ainfeld caught an important bug that once a user is on a tissue page, a click on a cell type link doesn't take the user to the tissue specific cell type page 🤦🏻‍♂️ My bad for missing that

Will add the feature and a test now!

@ainfeld
Copy link
Contributor

ainfeld commented Nov 30, 2023

Thanks so much @tihuan! I can now confirm there is no engineering updates needed for analytics. The updates needed given this change are all on my end.

@tihuan tihuan closed this as completed Dec 18, 2023
@tihuan
Copy link
Contributor

tihuan commented Dec 18, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data-viz Data Viz Team P0 Priority 0 - Critical, fix ASAP!
Projects
None yet
Development

No branches or pull requests

4 participants