-
Notifications
You must be signed in to change notification settings - Fork 605
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
Bug 1827361: Show the project selection list when a pinned item is shown for all projects #5176
Bug 1827361: Show the project selection list when a pinned item is shown for all projects #5176
Conversation
@jeff-phillips-18: This pull request references Bugzilla bug 1827361, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In 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/test-infra repository. |
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 an elegantly simple solution. I worry about the connectToPlural right now though. Looking around to see if I can find an alternative.
kindForReference, | ||
} from '@console/internal/module/k8s'; | ||
import { ErrorPage404 } from '@console/internal/components/error'; | ||
import { withStartGuide } from '../../../../public/components/start-guide'; |
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.
import { withStartGuide } from '../../../../public/components/start-guide'; | |
import { withStartGuide } from '@console/internal/components/start-guide'; |
<title>{kindObj.labelPlural}</title> | ||
</Helmet> | ||
<ProjectListPage title={kindObj.labelPlural}> | ||
{`Select a project to view the list of ${kindObj.labelPlural}`} |
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.
Nit
{`Select a project to view the list of ${kindObj.labelPlural}`} | |
Select a project to view the list of {kindObj.labelPlural} |
); | ||
}; | ||
|
||
export default connectToPlural(withStartGuide(ProjectSelectPage)); |
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.
connectToPlural
is deprecated. Let me see what an alternative would be.
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 only alternative I can see is if you thread the model
from getPinnedItems
(in perspective-nav.tsx) and use React-Router-Dom to
as an object and provide the state.
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 think its best to leave as is and someday when the connectToPlural is really removed it is replaced with a viable alternative
<Helmet> | ||
<title>{kindObj.labelPlural}</title> | ||
</Helmet> | ||
<ProjectListPage title={kindObj.labelPlural}> |
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.
Need to include the badge, if there is one.
<ProjectListPage title={kindObj.labelPlural}> | |
<ProjectListPage title={kindObj.labelPlural} badge={getBadgeFromType(kindObj.badge)}> |
ddf0cdf
to
9c04f51
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.
@christianvogt Would you be able to take a quick look here, I am still not super happy with the use of a deprecated function but the deprecation notice really should have listed an alternative.
I think Sam successfully fixed the issue where the conflict with Knative and the regular models occurred... so I suspect this will work fine.
loader: async () => | ||
( | ||
await import( | ||
'./components/ProjectSelectPage' /* webpackChunkName: "dev-console-buildconfigs" */ |
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.
update webpack chunk name.
'./components/ProjectSelectPage' /* webpackChunkName: "dev-console-buildconfigs" */ | |
'./components/ProjectSelectPage' /* webpackChunkName: "dev-console-projectselectpage" */ |
…own for all projects
9c04f51
to
10aa0a0
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.
Verified locally and works fine.
Waiting on a comment to #5176 (review) be resolved.
@andrewballantyne i think |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, christianvogt, jeff-phillips-18 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@jeff-phillips-18: All pull requests linked via external trackers have merged: openshift/console#5176. Bugzilla bug 1827361 has been moved to the MODIFIED state. In 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/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-3653
Analysis / Root cause:
Pinned items show the standard resources list page in the dev perspective
Solution Description:
Add a Page/Route plugin for resources in the dev perspective plugin to show the project select page when using all-namespaces.
/kind bug