browse table improvements (#3390) #3753
Conversation
|
Deploy preview for hasura-docs ready! Built with commit 2a1bb65 |
|
Review app for commit 4cfed0a deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
|
Review app for commit f62fbe1 deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
f62fbe1 to
728788a
Compare
|
Review app for commit 728788a deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
|
Review app for commit 8647e1a deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
|
Review app for commit 55d4903 deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
313acb8 to
f3581b8
Compare
|
Review app for commit f3581b8 deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
|
Review app for commit 46d7ff9 deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
rikinsk
left a comment
There was a problem hiding this comment.
Functionality seems to work great
Some UI feedback though
- The hamburger seems too big currently. You can use the same class used for the sort icon to keep it consistent. Ideally they should be generated via the same function that takes the icon type is input.
- When a column is collapsed, we should change the title of the expand icon to
Expand column "<column_name>". If multiple columns are collapsed and I need to expand one, it is hard to get the right one without guessing and expanding the columns one by one
|
Review app for commit e8e6dee deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
|
Review app for commit 4f290fa deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
|
Review app for commit 1b49b67 deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
…l-engine into browse-table-improvements
|
I wasnt quite happy with the hamburger as it makes the header look too crowded. So instead, I removed it and updated the header title to mention it. Also refactored the DragFoldTable to keep it generic for usage in other places. |
|
Review app for commit cf117ad deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
rikinsk
left a comment
There was a problem hiding this comment.
UX approved.
During code review please keep in mind that the DragFoldTable is a common component that should remain reusable e.g. the LS keys used should be params so that we can persist state for different DragFoldTables used in other pages as well
marionschleifer
left a comment
There was a problem hiding this comment.
Changelog approved.
| MAX_WIDTH = 600, | ||
| HEADER_PADDING = 62, | ||
| CONTENT_PADDING = 18, | ||
| HEADER_FONT = 'bold 16px Gudea', |
There was a problem hiding this comment.
Can these defaults come from constants?
There was a problem hiding this comment.
As these values are not relevant to any other function, they were defined as constants in the fn def itself and put into params so that they can be overridden if needed.
There was a problem hiding this comment.
@rikinsk my motivation for this comment was, because these constants are CSS properties, these values shouldn't be hard coded here if anywhere. Because CSS inconsistencies have a very explicit impact on the end user.
console/src/components/Common/TableCommon/foldableTable/index.jsx
Outdated
Show resolved
Hide resolved
console/src/components/Common/TableCommon/foldableTable/index.jsx
Outdated
Show resolved
Hide resolved
|
Review app for commit b2ed957 deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
|
@rikinsk don't merge it yet. I noticed a bug with foldable columns. Edit: fixed. |
|
Review app for commit 2a1bb65 deployed to Heroku: https://hge-ci-pull-3753.herokuapp.com |
|
Review app https://hge-ci-pull-3753.herokuapp.com is deleted |
Affected components
What was done
1. Don't show columns that are already chosen for sort in the dropdown.
This would prevent following scenario:
How it works:
2. Collapsed columns state was made persistent by storing it in local storage.
3. Fix styles for all columns collapsed state.
Before:
After:
4. Made columns order persistent by storing reorders data in local storage.
It also makes some cleanup:
Afrom position 2 to 3.A: 2 -> 3)A: 3 -> 2)A: 3 -> 2) reorder will be no longer in local storage, since columnsAcame back at its original place.Related issues