Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Replace @mui/x-data-grid with material-react-table #49

Merged
merged 21 commits into from
Feb 5, 2024
Merged

Replace @mui/x-data-grid with material-react-table #49

merged 21 commits into from
Feb 5, 2024

Conversation

zmc
Copy link
Member

@zmc zmc commented Aug 11, 2023 •
edited
Loading

This is intended to fix #48 and #61

Current status: Porting complete. Expansions are implemented!

@render
Copy link

render bot commented Aug 11, 2023

Your Render PR Server URL is https://pulpito-ng-pr-49.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-cjbannfdb61s738ng31g.

@netlify
Copy link

netlify bot commented Aug 11, 2023 •
edited
Loading

✅ Deploy Preview for pulpito ready!

Name Link
🔨 Latest commit ce7a60e
🔍 Latest deploy log https://app.netlify.com/sites/pulpito/deploys/65bd5685bf6d760008463a24
😎 Deploy Preview https://deploy-preview-49--pulpito.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

This was referenced Oct 25, 2023
Open
Closed
@zmc zmc force-pushed the mrt branch 2 times, most recently from b307b05 to 0c058f8 Compare December 12, 2023 01:08
@zmc zmc changed the title WIP: Replace @mui/x-data-grid with material-react-table Replace @mui/x-data-grid with material-react-table Dec 12, 2023
@zmc zmc force-pushed the mrt branch 2 times, most recently from 93a3f11 to 802e5fd Compare December 12, 2023 02:23
@zmc zmc marked this pull request as ready for review December 12, 2023 02:23
@VallariAg
Copy link
Member

Wow, this looks really amazing! I especially love this cool feature right here:
image

@zmc zmc force-pushed the mrt branch 5 times, most recently from f8e8c21 to 5cb66c0 Compare December 19, 2023 22:01
VallariAg
VallariAg reviewed Dec 20, 2023
View reviewed changes
Copy link
Member

@VallariAg VallariAg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/queue page seems to be giving some warning/error on development environment:

Uncaught TypeError: setter is not a function
    at Object.onColumnFiltersChange (index.tsx:210:5)
    at table.setColumnFilters (Filters.ts:584:21)
    at column.setFilterValue (Filters.ts:470:13)
    at handleClear (MRT_FilterTextField.tsx:183:14)
    at MRT_FilterTextField.tsx:206:9
    at commitHookEffectListMount (react-dom.development.js:23150:26)
    at invokePassiveEffectMountInDEV (react-dom.development.js:25154:13)
    at invokeEffectsInDev (react-dom.development.js:27351:11)
    at commitDoubleInvokeEffectsInDEV (react-dom.development.js:27330:7)
    at flushPassiveEffectsImpl (react-dom.development.js:27056:5)

Also, I'm not sure why but particular node pages like this https://pulpito-ng-pr-49.onrender.com/nodes/smithi001.front.sepia.ceph.com/ have tables which are not 100% in width:
image

src/components/RunList/index.tsx Show resolved Hide resolved
src/components/NodeList/index.tsx Outdated Show resolved Hide resolved
@zmc
Replace @mui/x-data-grid with material-react-table
3810cc4 
Fixes: #61

Signed-off-by: Zack Cerza <[email protected]>
@zmc
npm uninstall @mui/x-data-grid
ea584d1 
Signed-off-by: Zack Cerza <[email protected]>
@zmc
Move theme definition to lib/
4c05030 
This adds our former DataGrid theming into the theme itself.

Signed-off-by: Zack Cerza <[email protected]>
@zmc
Fix dark mode toggle and persist setting
e54d51a 
Signed-off-by: Zack Cerza <[email protected]>
@zmc
JobList: Add expandable failure reasons
0fef852 
Fixes: #48

Signed-off-by: Zack Cerza <[email protected]>
@zmc
Fix lint errors from previously-untested files
e73a18a 
Signed-off-by: Zack Cerza <[email protected]>
VallariAg
VallariAg reviewed Jan 25, 2024
View reviewed changes
src/components/NodeList/index.tsx Outdated Show resolved Hide resolved
src/components/RunList/index.tsx Outdated Show resolved Hide resolved
src/components/RunList/index.tsx Show resolved Hide resolved
VallariAg
VallariAg reviewed Jan 25, 2024
View reviewed changes
Copy link
Member

@VallariAg VallariAg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Light mode colours look beautiful - thanks for the UI facelift! 🌟

src/lib/table.ts Outdated Show resolved Hide resolved
src/components/JobList/index.tsx Show resolved Hide resolved
src/components/JobList/index.tsx Outdated Show resolved Hide resolved
src/pages/StatsNodesJobs/index.tsx Show resolved Hide resolved
@zmc
NodeList: Rework column auto-hiding
a0883a6 
The name column will be shown when >1 nodes are being displayed
The arch and machine_type columns will be shown when there are more than one
unique value in the dataset

Signed-off-by: Zack Cerza <[email protected]>
@zmc
Queue: Fix props and table options
0331e5f 
And use TypeScript.

Signed-off-by: Zack Cerza <[email protected]>
@zmc zmc force-pushed the mrt branch 2 times, most recently from c1d551c to 636c8d4 Compare January 25, 2024 23:03
@zmc
table: Pad and color cells
dfce1e1 
Cells containing SVGs keep their original padding.

Signed-off-by: Zack Cerza <[email protected]>
@zmc
Copy link
Member Author

zmc commented Jan 25, 2024

Thanks so much for the review @VallariAg! I've pushed some updates.

Re: the blue rows, I think queued and waiting being blue is okay, since neither have actually run yet. Runs shouldn't have an unknown status, but that's clearly a bug we need to track down in paddles. I am not sure if having no background color is better, though.

@VallariAg
Copy link
Member

the blue rows, I think queued and waiting being blue is okay, since neither have actually run yet.

Ah okay, understood!
Also, for now, I have created an issue on paddles for the "unknown" status: ceph/paddles#122

The PR looks really great! Almost done, I just had 2 final comments that I would like your opinion on:
#49 (comment) and #49 (comment)

@zmc
JobList: Drop hover text
46d3408 
Now that we have expandos, we don't need the tooltips.

Signed-off-by: Zack Cerza <[email protected]>
@VallariAg VallariAg linked an issue Feb 2, 2024 that may be closed by this pull request
Run Page: DataGrid's column filters not working #70
Closed
@zmc
Node: Sort jobs chronologically
f6f9a7a 
Signed-off-by: Zack Cerza <[email protected]>
@zmc
JobList: Fix node count for queued jobs
99119d7 
Signed-off-by: Zack Cerza <[email protected]>
@zmc
Update vite
ce7a60e 
To pull in Vite PR #14756 to fix routing inconsistencies between dev and prod.

Signed-off-by: Zack Cerza <[email protected]>
VallariAg
VallariAg approved these changes Feb 3, 2024
View reviewed changes
Copy link
Member

@VallariAg VallariAg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks perfect, thank you so much for porting this!

@zmc zmc merged commit 21b8c46 into main Feb 5, 2024
7 checks passed
@zmc zmc deleted the mrt branch February 5, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VallariAg VallariAg VallariAg approved these changes

@kamoltat kamoltat Awaiting requested review from kamoltat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run Page: DataGrid's column filters not working Missing "expand" feature on Run page
2 participants
@zmc @VallariAg