-
Notifications
You must be signed in to change notification settings - Fork 667
CONSOLE-4529: POC PodList using DataView PF extension #14897
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
Conversation
|
@Hyperkid123: This pull request references CONSOLE-4529 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.19.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hi @Hyperkid123. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@Mylanos: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
beae73a to
6107c3d
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Hyperkid123 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
6107c3d to
6a816c1
Compare
| * The sorting logic was copied over from public/components/factory/Table/VirtualizedTable.tsx file | ||
| * For the most part it is 1:1 | ||
| * column removal based on screen size was omitted from POC | ||
| * the columnShift attribute was omitted from the POC |
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 appears columnShift was added for the local storage operator plugin, which has since moved out of the repo, and I don't see any other instances that are using it, so I think it is ok to omit for now.
| /** | ||
| * The sorting logic was copied over from public/components/factory/Table/VirtualizedTable.tsx file | ||
| * For the most part it is 1:1 | ||
| * column removal based on screen size was omitted from POC |
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.
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 more I continue to think about this, the more I think we need to engage UXD and PatternFly here. We have some very wide tables that will be unusable without a solution. PatternFly's sticky column seems like an obvious choice.
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.
And if we do go the sticky column route, we could conceivably drop the maximum number of columns (currently 9) that can be active when column management is in use?
Also wondering if column management shouldn't be enabled on all tables? We've had requests for columns to be resizable, but PatternFly doesn't yet support that. Being able to manage the columns may help in that regard?
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.
You can still use the original column "hiding" mechanism. You have to use different elements as your width baseline.
About the sticky columns, you can use any valid PF prop on your columns/cells.
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.
You can still use the original column "hiding" mechanism. You have to use different elements as your width baseline.
Are you referring to column removal from right to left as the viewport shrinks? If so, can you clarify what you mean by "use different elements as your width baseline"?
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.
Yes. I mean that. You can still do that, Its all about what data you feed to the data view. If you used the virtualized table as your element where you computed the widht and visible elements, now you would do that with the data view columns instead.
| }, {}); | ||
| const debouncedOnFilterChange = useDebounceCallback(onFilterChange, 500); | ||
| const debouncedSetSearchParams = useDebounceCallback(setSearchParams, 500); | ||
| // Currently there is a big that will only return the first item from the query, even though there are multiple items for one group |
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.
@Hyperkid123, can you clarify what you mean here? I'm not clear on what you're describing.
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.
Oh sorry. There was a bug. I fixed it here: patternfly/react-data-view#395
Bug not big 🙂
| const debouncedOnFilterChange = useDebounceCallback(onFilterChange, 500); | ||
| const debouncedSetSearchParams = useDebounceCallback(setSearchParams, 500); | ||
| // Currently there is a big that will only return the first item from the query, even though there are multiple items for one group | ||
| const { onSetFilters, filters: dataViewFilters } = useDataViewFilters({ |
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.
clearAllFilters is missing here. I added it and passed it to <DataViewToolbar>, but it's not working. Any ideas?
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.
Thinking this is a PatternFly bug.
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.
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, @Hyperkid123. The clearAllFilters bug persists with v6.2.0 that includes patternfly/react-data-view#395.
When using clearAllFilters, the filters don't get unset.
Screen.Recording.2025-05-27.at.9.01.47.AM.mov
When removing the individual filter, the filter does get unset.
Screen.Recording.2025-05-27.at.9.02.08.AM.mov
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.
See #15057
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.
OK I don't think its something specific to data view, I think it might be something with how I integrated it into the OCP. I'll poke around for a bit now.
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.
Well no wonder, I did not add the clear all callback at all 🙂 . It should be a prop on this line: https://github.com/openshift/console/pull/14897/files#diff-73803de448a478f334f4a0c2b0c1b23bbc51a1835f6dcbb92b0afb74f6f956b0R752
Let me add a sample.
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.
@rhamilto ok I've added the callback here: dd3ecf8
its almost working, I think with how the filtering is wired in this component in general it will need some extra bits than just using the standard callback. By iterating over the actual filters that are managed outside of data view and clearing them I am able to use that clear all and it clears the data.
Now that is where some OCP expertise is required as I did not want to start changing the data state management for the filters. Right with the POC the filters are being managed in two places. Which is most likely the reason why the standard clear all filters callback from data view does not do the job.
You will probably have to chose one or the other and not mix them.
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.
Ok. I cobbled together a POC that only uses data view filters and everything works as expected.
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.
Thats great to hear!
|
@Hyperkid123: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
Jira: https://issues.redhat.com/browse/CONSOLE-4529
Replacing virtualized table with
DataViewcomponent from PR extension.Out of scope
Data view also provides a toolbar, however, toolbar implementation is not used here yet. It should be a follow-up once some design differences are addressed and potential issues with state management using mutable data structures.
Changes
Implement the pod
PodListtable using theDataViewtable component. Some code is copied and pasted from the virtualized table to prevent cyclic import and regressions to existing code.The component re-uses almost all existing code. Most of the work is related to mapping current data to the required
DataViewformat.The infinite scroll table is replaced with a paginated table.