-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
✨ Data catalog client #3858
✨ Data catalog client #3858
Conversation
Quick links (staging server):
Login:
SVG tester:Number of differences (default views): 4523 (f7dd60) ❌ Edited: 2024-09-17 23:09:56 UTC |
This PR has had no activity within the last two weeks. It is considered stale and will be closed in 3 days if no further activity is detected. |
19f8c17
to
8c7a033
Compare
f571a36
to
9fd41fd
Compare
8c7a033
to
08bc9e4
Compare
Oh, weird! Maybe because the bake failed? Lemme try again.. |
710a48b
to
c5a4896
Compare
08bc9e4
to
9d541eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! I don't see any major problems, but I left a few comments for possible small improvements we might still do.
Also, could you add docs on how to run the catalog locally?
Areas of possible improvements/potential follow-ups (some of which we already discussed) for the future:
- Handling out-of-order/slow requests and request cancellation when fetching data - I'm mostly worried about this, but I'm not sure if it's gonna really be a problem in practice
- Ordering query params when creating the cache key, so their order won't matter, and they'd use a single cache key
- Reusing existing checkbox component
- Storing search state in the URL only, instead of syncing between URL and React state
Possible 3rd party solutions to some of these problems:
- React Router hooks to use URL for state and relatedly avoid handling mutable global history state + global event listeners
- react-query for data fetching, cache, request cancellation etc.
import React from "react" | ||
import cx from "classnames" | ||
|
||
// 💀 Beware! Spooky skeletons for the data catalog 💀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made me smile 😄
Staging
Figma
Changes
/charts
page/charts
to/data
useFocusTrap
hook