CNV-29444: Feature guide user to pipelines #1911
Conversation
|
@pcbailey: This pull request references CNV-29444 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 epic to target either version "4.16." or "openshift-4.16.", but it targets "CNV v4.16.0" instead. 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 openshift-eng/jira-lifecycle-plugin repository. |
pcbailey
force-pushed
the
feature-guide-user-to-pipelines
branch
from
19400f9
to
411ab57
Compare
|
@pcbailey: This pull request references CNV-29444 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 epic to target either version "4.16." or "openshift-4.16.", but it targets "CNV v4.16.0" instead. 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 openshift-eng/jira-lifecycle-plugin repository. |
pcbailey
changed the title
pcbailey
changed the title
pcbailey
force-pushed
the
feature-guide-user-to-pipelines
branch
from
411ab57
to
cefe22d
Compare
pcbailey
changed the title
| import { getDisabledQuickStarts, QuickStart } from '@patternfly/quickstarts'; | ||
|
|
||
| const useQuickStarts = (filterDisabledQuickStarts = true): WatchK8sResult<QuickStart[]> => { | ||
| const preferredLanguage = useTranslation().i18n.language; |
There was a problem hiding this comment.
its nicer to deconstruct on left side then the right when using hooks
There was a problem hiding this comment.
I'm not sure what you mean. Could you explain a bit more and explain why?
Also, the code in this file is from console. I copied it over in my attempt to get quick starts working. If we end up having to use it I'll clean it up.
|
|
||
| if (filterDisabledQuickStarts) { | ||
| const disabledQuickStarts = getDisabledQuickStarts(); | ||
| disabledQuickStarts.forEach((quickStartName) => delete groupedQuickStarts[quickStartName]); |
There was a problem hiding this comment.
instead of delete and modifing the obj u can filter and get a new obj. from running on one obj u modify a different obj. better to work on groupedQuickStarts and filter on it
| ).toUpperCase(); | ||
|
|
||
| if (quickStartLanguage === preferredLanguage && quickStartCountry === preferredCountry) { | ||
| return quickStart; |
There was a problem hiding this comment.
here u are returning the first match, and all other checks u are returning the last match?
There was a problem hiding this comment.
This code is also from console. I'll clean it up as well if we end up using it.
| if (quickStartLanguage === preferredLanguage) { | ||
| if (!quickStartCountry && !sameLanguageWithoutCountry) { | ||
| sameLanguageWithoutCountry = quickStart; | ||
| } else if (quickStartCountry && !sameLanguageWithAnyCountry) { | ||
| sameLanguageWithAnyCountry = quickStart; | ||
| } | ||
| } | ||
| if (quickStartLanguage === 'en') { | ||
| if (quickStartCountry === preferredCountry && !fallbackLanguageSameCountry) { | ||
| fallbackLanguageSameCountry = quickStart; | ||
| } else if (!quickStartCountry && !fallbackLanguageNoCountry) { | ||
| fallbackLanguageNoCountry = quickStart; | ||
| } else if (!fallbackLanguageAnyCountry) { | ||
| fallbackLanguageAnyCountry = quickStart; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please consider changing the mechanism to a more readable one, this if, if else if isn't something we usually do.
...atalog/CreateFromInstanceTypes/components/BootableVolumeEmptyState/BootableVolumeOSIcons.tsx
Show resolved
Hide resolved
| <div className="bootable-volumes-pipelines-hint"> | ||
| <img alt="os-icon" className="bootable-volumes-pipelines-hint__icon" src={rhelIcon} /> | ||
| <Trans ns="plugin__kubevirt-plugin" t={t}> | ||
| Interested in using a <b>RHEL Bootable Volume</b>? Click{' '} | ||
| <AddBootableVolumeLink loadError={loadError} /> to get started. | ||
| </Trans> | ||
| </div> |
There was a problem hiding this comment.
there is a lot of repeative code here , in the 3 return statements, can create a new component that take a some props and create this.
There was a problem hiding this comment.
Possibly. They're not exactly alike though and the main changes are all in the Trans block. I'll take a look and see if I can come up with something worth using.
...catalog/CreateFromInstanceTypes/components/BootableVolumeList/hooks/useBootableVolumeOSes.ts
Show resolved
Hide resolved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: metalice, pcbailey 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 |
📝 Description
This PR adds elements to guide users to use pipelines for bootable volume creation.
Jira: https://issues.redhat.com/browse/CNV-29444
🎥 Screenshots