-
Notifications
You must be signed in to change notification settings - Fork 181
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
Persist table column sizes #45
Conversation
🦋 Changeset detectedLatest commit: abc4e3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Looks awesome, thanks so much for the contribution, really excited for it!
I think the biggest blocker is the issue around the resize reset button not working due to leaving the column width out of the memo dependency, but adding it as a memo dependency breaks resizing.
Otherwise would love to see the reset button moved over as a selectively shown icon instead as well just for UI purposes :)
packages/app/src/LogTable.tsx
Outdated
setColumnSizeStorage({ ...columnSizeStorage, ...state }); | ||
}; | ||
|
||
const reactTableProps = (): TableOptions<any> => { |
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.
I think it might be good to memo this value right?
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.
Good idea, added useMemo()
@@ -377,7 +390,7 @@ export const RawLogTable = memo( | |||
{info.getValue<string>()} | |||
</span> | |||
), | |||
size: 150, | |||
size: columnSizeStorage[column] ?? 150, |
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.
I saw that the linter wasn't happy about this missing from the dep array which I'd like to have fixed, but noticed that when added the resizing no longer worked.
The side-effect of this as it is today is if you hit reset column widths, it doesn't actually reset column widths until the page is refreshed which is a pretty odd experience.
I'm wondering if you have any ideas on how to workaround those problems?
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.
Good call out, I have added columnSizeStorage
to the deps array and the resize seemed to work fine for me. Is there set of steps to recreate it?
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.
I must've been doing something weird then - this PR works as is which is awesome!
packages/app/src/LogTable.tsx
Outdated
@@ -396,6 +409,16 @@ export const RawLogTable = memo( | |||
</span> | |||
</span> | |||
)} | |||
<span> |
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.
I was thinking that the reset should live as a reset icon next to the gear icon on the top right of the table so it won't be too much visual clutter. Additionally I'd love to see it only when the table has column persistence enabled (with a tableId
and also only if there's actually resized columns.
Was thinking something like this where the bi-gear-fill
icon is currently:
{headerIndex === headerGroup.headers.length - 1 && (
<div
className="d-flex align-items-center"
style={{
position: 'absolute',
right: 8,
top: 0,
bottom: 0,
}}
>
{tableId != null &&
Object.keys(columnSizeStorage).length > 0 && (
<div
className="fs-8 text-muted-hover"
role="button"
onClick={() => setColumnSizeStorage({})}
title="Reset Column Widths"
>
<i className="bi bi-arrow-clockwise" />
</div>
)}
{onSettingsClick != null && (
<div
className="fs-8 text-muted-hover ms-2"
role="button"
onClick={() => onSettingsClick()}
>
<i className="bi bi-gear-fill" />
</div>
)}
</div>
)}
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.
Good catch, didn't think of this earlier. Fixed in latest commit.
…ton on default, change resize to icon
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.
looks awesome, sorry for the turnaround but I'm excited for this!!
@@ -377,7 +390,7 @@ export const RawLogTable = memo( | |||
{info.getValue<string>()} | |||
</span> | |||
), | |||
size: 150, | |||
size: columnSizeStorage[column] ?? 150, |
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.
I must've been doing something weird then - this PR works as is which is awesome!
@JeyRathnam sorry one more thing, could you run That way it'll make it into the changelog and also bump versions appropriately :) |
I have added the changeset commit. Changeset is a cool tool, learned something new! |
@JeyRathnam really really sorry for the late review - didn't get a ping for some reason that this was ready again :( Just enabled automerge to do the rest, thank you so much for contributing!!! 🙏 |
fixes #27