-
Notifications
You must be signed in to change notification settings - Fork 16
feat: Column sources for ui.table formatting #1010
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
feat: Column sources for ui.table formatting #1010
Conversation
| const stableArray = useMemo(() => array, [...array]); | ||
| return stableArray; | ||
| const stableArray = useRef<T[]>(array); | ||
| if (!array.every((v, i) => v === stableArray.current[i])) { |
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.
Curious what this improves over the previous version?
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.
The previous version was actually a React error if the length of the array changed. useMemo dependency array is supposed to be a constant length, so this should allow for variable length arrays.
I will probably be replacing this with a useDeepEquals in a future PR that also handles objects
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.
Ah makes sense. Forgot about the length nuance. With that in mind, you could optimize this a bit more by checking if the length has changed before traversing the array.
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.
| if (!array.every((v, i) => v === stableArray.current[i])) { | |
| if (array.length !== stableArray.length || !array.every((v, i) => v === stableArray.current[i])) { |
bmingles
left a comment
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.
1 suggestion on error message. Otherwise, code looks good. Verified the String + non-string cases.
bmingles
left a comment
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.
1 more suggestion to optimize the array memoization
bmingles
left a comment
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
Fixes #984
Allows all string format values to instead specify a column as the source. Ensures the column is always fetched and throws if the column is not a string type (can cause some really bad performance issues if value formatting is invalid). Also modified to just resolve all theme colors since this change could lead to unresolved theme colors in a column source.
The example in the docs or this example shows the feature working. Change the String cast on line 6 to non-string to test the error is thrown and displayed.
This example uses an input table joined to the table so you can adjust the formatting colors via input table (something Raffi asked about specifically)