-
Notifications
You must be signed in to change notification settings - Fork 196
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
frontend: Add new Table component and use it in ResourceTable #1921
Conversation
b30a3c6
to
0afbac3
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.
It's looking good. I left a few comments. The ResourceTable is something we export to plugins, so we need to make sure it's backwards compatible (including the exported useThrottle
hook that you moved). I know this is complicated to do with so many fields, so if there's another suggestion (new component name that fits right), I'm open to that.
Otherwise, we do try to keep commits atomic, so if there are changes that should be in other isolated commits, let's move them there.
I've reverted some name changes to keep ResourceTable backwards compatible. I missed that it was a common component. The only partly incompatible thing is the |
366be58
to
6c2dd54
Compare
This will be annoying to developers who need to rebuild but cannot yet update that part of the table (maybe they can be time-pressed). |
Good point, I went with the latter option. Catching places where JSX getter is used is unreliable, even in our codebase jsx is sometimes typed as any, so I went with introducing a new property getValue. |
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 have noticed a few issues with the table replacement:
Column chooser considers a column that has just a context menu:
Long cell values are not ellipsizing or wrapping, just expanding the table:
The age column has the column controls on the left (probably because we align this columns name to the right but what we really need is for the column to occupy the minimum width it needs)
The filters/search in the table shouldn't conflict with the ones in the section header:
If not super complicated I think we should for now keep the section filter but make the search there be used for the table search (which is much better than what we have). The namespace filter should just show the section namespace filtering (we need to study how to move the namespace to a more global way).
If I find more cases, I will comment them.
d0ac626
to
c644f84
Compare
Fixed, columns without a label will not be shown in the column picker.
Fixed, wrapping now behaves like it used to
Age column is now narrow, I've kept the right alignment, I think it looks better that way
Implemented the changes we discussed. The section header will now only have namespace switcher that is displayed without a toggle, search is now handled only inside the table. |
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.
Thanks @sniok . Here are some comments:
translations' *_old.json files are not to be committed.
Age column:
- Even though it's nice for the values to be aligned to the right, the column shows the controls to the left of it:
Nodes:
Namespace Header filter:
Column chooser popover:
- Has a "Hide All" option. Not sure if there's a quick way to remove this option, but the option doesn't make a lot of sense IMO.
Please do not remove any options that are exported to plugins, instead mark them as deprecated (we have some in the source code).
a5ba031
to
f46cda3
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.
Nice work.
I left a few questions and notes, but haven't tested it yet.
Could you please let me know what testing you have done?
A nit on the commit messages. We use a capital letter for the verb. Rather than just "frontend:" it's good to put the component(s) that are affected. Probably the main components can be given as context, or if things are unrelated they can be moved to other commits.
Just manual, looked through all of the tables side by side with current headlamp version. Checked if all the example plugins are building without an error |
@sniok I have pushed this style change just for tracking it. Today I will bring this to the design team and check whether we keep this design change or do something else. Let's wait for that before we merge the commit + update snaptshots. I have found the following problems when looking further: Maybe if we enable the column resizing feature, it will prevent these issues? |
I noticed now that this table may not be the new one. |
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.
We need to remove the old.json translation files + we cannot add the snapshots to a fixup commit because when it's squashed to where it belongs, the snapshots may include conflicts. So usually we need to squash it and then regenerate the snapshots for each commit.
73f1609
to
db12cd7
Compare
namespace filter was reverted. went through each commit and regenerated snapshots |
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.
A couple of last minute things to check but otherwise looks good to me.
Can you please rebase with main when you have a chance? |
The gh interface is pretty bad with these big PRs. It's super slow, and I don't get notified when a convo is resolved. To help with this, can we please leave convos open until they are resolved by note opener. This way we don't have to search through 90 of them whilst my CPU fans are on 100%. |
Made Table exported as default and rebased with main branch |
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.
frontend/src/components/common/Resource/ResourceTableColumnChooser.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
f2ce118
to
860d7cd
Compare
Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
This commit adds a new Table component based on the material-react-table It will be used for the places where we need more advanced table features and where SimpleTable is not enough. This tables aim to have all the existing SimpleTable features for compatibility reasons. Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
Replaces SimpleTable with the new Table component. Out of the box ResourceTable will enable sorting and filtering on all columns. To provide that functionality 'getter' in the column definition has to provide a plaintext value (but will keep working if it returns jsx for compatibility reasons). e2e and snapshot tests were also updated Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
Remove global filter state and filter input from the SectionFilterHeader New Table component now has filtering built-in so we don't need filter in the header Also remove uses of useFilterFunc in places where Table can already filter by the columns Replace SimpleTable with Table in Notifications page Implement grid layout in new Table that will work the same way SimpleTable works Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
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.
🎉🎈
We want more features in our tables. Stuff like sorting, filtering, selection, actions, etc. This is where the new
Table
component comes in andSimpleTable
will remain simple.material-react-table (MRT) was chosen because it uses the same ui framework, looks good, has all the feature we will need, MIT licensed, based on headless tanstack table and very customizable.
Table
component is mostly a wrapper around MRT with some sensible defaults and additional app specific behaviour (like storing page state in url). I kept most of the props as aliases to the MRT props so it can be extensible without introducing any plumbing.In the scope of this PR I also updated ResourceTable component to use the new table. The only change from the 'outside' is that each column needs to provide getter and renderer functions, first is needed for filtering and sorting, second one is for displaying. Getter needs to return a string and since most of the existing column getters were simple functions there wasn't many changes there. In those cases where existing getter returned JSX node it was renamed to render and a simple getter was added, representing plain value.
While not in the scope of this PR there are features that we can implement now using the new Table: row selection, actions column, multi-select for the filtering, selection actions (like deleting all selected Pods).
Fixes #1646 #1641 and unblocks #1006 #1640
I'm still checking if I didn't break anything and ported the existing functionality properly but if you want you can leave any comments or suggestions