-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
refactor: useListViewResource hook for charts, dashboards, datasets #10680
Conversation
2895b7b
to
2b0ee57
Compare
oops |
a60d947
to
b189dc9
Compare
99a4587
to
260b22a
Compare
Codecov Report
@@ Coverage Diff @@
## master #10680 +/- ##
==========================================
- Coverage 59.53% 59.39% -0.15%
==========================================
Files 360 360
Lines 23173 23173
==========================================
- Hits 13796 13763 -33
- Misses 9377 9410 +33
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
{ | ||
accessor: 'description', | ||
hidden: true, | ||
disableSortBy: true, |
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.
nit unrelated to this PR, and out of scope for this change: it would be good to make this a positive setting rather than a negated setting, so rather than disableSortBy: true
, it could be more readable to have the option sortingEnabled: false
.
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.
originally the property was called "sortable" however there were a few type errors creeped up so I opted to use react-table's included property https://react-table.tanstack.com/docs/api/useSortBy#table-options
If time permits I'll do a second pass at trying to clean up the columns config api.
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.
Understood. Please don't burn too many cycles on it given that it maps onto a 3rd party library's naming. Just a passing thought.
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.
LGTM! You've done a great job with this... but man, that Favorites star does a LOT of work. In a more perfect world, the favorite status would just be part of the chart/dash array, so you don't have to do all these API calls and whatnot.
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
ADDITIONAL INFORMATION