-
Notifications
You must be signed in to change notification settings - Fork 667
CONSOLE-4667: Add ResourceDataView component and related code #15375
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
CONSOLE-4667: Add ResourceDataView component and related code #15375
Conversation
|
@vojtechszocs: This pull request references CONSOLE-4667 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 story to target the "4.20.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. |
a07ff6e to
a652a44
Compare
|
@vojtechszocs: This pull request references CONSOLE-4667 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 story to target the "4.20.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. |
|
/retest |
frontend/packages/console-app/src/components/data-view/useResourceDataViewFilters.ts
Show resolved
Hide resolved
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 few observations from when I applied this code in openshift/console-crontab-plugin#45
frontend/packages/console-app/src/components/data-view/useResourceDataViewData.tsx
Outdated
Show resolved
Hide resolved
| sortFunction: sort, | ||
| props: { | ||
| className: props.classes, | ||
| sort: { |
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.
This results in the actions column getting sorting functionality, which is not desired. Sorting should be optional on a per column basis.
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 I believe sorting is broken for all but the first column, but even there the header doesn't correctly indicate the sort direction.
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.
openshift/console-crontab-plugin@87da69a makes sorting optional.
Still need to troubleshoot the sorting issues in useResourceDataViewSort.
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.
Note that the hard-coded sortBy object was present in the original DataView PoC code:
- CONSOLE-4529: POC PodList using DataView PF extension #14897 - file
frontend/public/components/data-view-poc/DataViewPodList.tsxline 489 - [WIP] Data view poc #15057 - file
frontend/public/components/data-view-poc/DataViewPodList.tsxline 513
sortBy: {
defaultDirection: SortByDirection.asc,
direction: SortByDirection.asc,
index: 0,
}I think that fixing sorting bugs (beyond any immediate issues) can be done as a follow-up.
This PR was supposed to introduce a reusable ResourceDataView component by refactoring and restructuring the original DataView PoC code.
a652a44 to
e1cc748
Compare
frontend/packages/console-app/src/components/data-view/useResourceDataViewData.tsx
Outdated
Show resolved
Hide resolved
|
includes the correct fix so the filters dropdown correctly honors |
|
Oye. One more change. |
|
It occurs to me those patches won't apply cleanly since my directory structure in the CronTab plugin doesn't match, but at least they're there. |
I think this patch should work (it should contain all the changes we discussed yesterday). |
|
@vojtechszocs: This pull request references CONSOLE-4667 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 story to target the "4.21.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. |
|
@vojtechszocs: This pull request references CONSOLE-4667 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 story to target the "4.21.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. |
|
@vojtechszocs, you need to run |
|
code approval: |
|
@rhamilto: GitHub didn't allow me to assign the following users: jseseCCS. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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. |
| /** | ||
| * Console DataView component based on PatternFly DataView. | ||
| */ | ||
| export const ResourceDataView = < |
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.
Shall we add unit test for the new component?
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.
@yapei I think we can skip adding unit tests for this component at this point in time.
The main goal of this PR was to refactor original DataView PoC code from #14897 and #15057 without putting this component into any use. This is the initial step towards using PatternFly DataView code in Console.
Any immediate issues found by @rhamilto via CronTab plugin testing should be covered in this PR. Once this PR merges, the next step should be putting this component into use on select Console data tables and resolve any other issues we find along the way.
Once we're confident about the API (props) for this component, we can add unit tests for it.
|
@rhamilto Updated i18n - see the last commit. |
|
/lgtm Will address the remaining issues in 08a353c or additional follow ons. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Adding labels since this is foundational code that is not yet in use. Approvals will be done in #15469 where this code is applied. /label docs-approved |
|
/test okd-scos-e2e-aws-ovn |
|
@yapei, can we punt on the unit test for the time being? |
If so, can you add the remaining labels ( |
|
@vojtechszocs: This pull request references CONSOLE-4667 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 story to target the "4.21.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. |
|
@yapei: This PR has been marked as verified by 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. |
|
/test frontend |
|
@vojtechszocs: all tests passed! 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. |
This PR follows up on #14897 and #15057 by introducing a reusable
ResourceDataViewcomponent and related code.Existing resource list tables are not modified; there will be follow-up PRs to put this component into use.
Thanks to @rhamilto for testing the new component in CronTab plugin so we can fix any immediate issues.