-
Notifications
You must be signed in to change notification settings - Fork 32
Update to installation process #2130
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh 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 |
<EmptyStateFooter> | ||
<EmptyStateActions> | ||
<Button | ||
variant="primary" |
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.
Provided we have the ButtonVariant
enum, we should use it in all places for code consistency, otherwise there is no point in having such enum (using it in some places and not in others is inconsistent).
A good improvement is to create a custom rule in the linter that detects these attributes without proper enum usage.
@@ -0,0 +1,10 @@ | |||
.odf-storage-cluster-create-modal__setup-card { | |||
max-width: 25em; |
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.
Just to confirm: is em
chosen due to parent? em
is tied to the parent element while rem
is tied to the root.
9a46636
to
d75b673
Compare
Adds support for new modal Adds support for redirection to new installation flow Removes deployment type selection in cluster creation wizard Removes external system seleciton flow in cluster creation wizard Signed-off-by: Bipul Adhikari <[email protected]>
d75b673
to
6bff752
Compare
<> | ||
<PageHeading title={getPageTitle(location.pathname, t)} /> |
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.
<> | |
<PageHeading title={getPageTitle(location.pathname, t)} /> | |
<> | |
<Helmet> | |
<title>{getPageTitle(location.pathname, t)}</title> | |
</Helmet> | |
<PageHeading title={getPageTitle(location.pathname, t)} /> |
LGTM. Please add screenshots. @SanjalKatiyar to tag. |
@bipuladh: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@@ -580,11 +584,11 @@ | |||
"Data Foundation does not support HDD disks as local devices. Select SSD if you plan to use Data Foundation.": "Data Foundation does not support HDD disks as local devices. Select SSD if you plan to use Data Foundation.", | |||
"Data Foundation endpoint": "Data Foundation endpoint", | |||
"Data foundation must be {{version}} or above.": "Data foundation must be {{version}} or above.", | |||
"Data Foundation simplifies persistent storage and data services across your cloud-native infrastructure": "Data Foundation simplifies persistent storage and data services across your cloud-native infrastructure", |
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.
across your cloud-native infrastructure
isn't this wrong ?? we have non-cloud use cases as well.
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.
even if microcopy is pending, IMO we shouldn't add incorrect text, we will get bugs later otherwise.
"type": "console.navigation/href", | ||
"properties": { | ||
"id": "storage-overview", | ||
"insertBefore": "odfdashboard", |
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.
we won't be having odfdashboard
anymore right ?? It's replaced by storage-overview
IIUC.
"id": "storage-object", | ||
"insertAfter": "storage-overview", | ||
"section": "storage", | ||
"name": "%plugin__odf-console~Obejct Storage%", |
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.
"name": "%plugin__odf-console~Obejct Storage%", | |
"name": "%plugin__odf-console~Object Storage%", |
"type": "console.navigation/href", | ||
"properties": { | ||
"id": "storage-object", | ||
"insertAfter": "storage-overview", | ||
"section": "storage", | ||
"name": "%plugin__odf-console~Obejct Storage%", | ||
"href": "/odf/mcg" | ||
}, | ||
"flags": { | ||
"required": ["ODF_MODEL", "ODF_ADMIN"] | ||
} | ||
}, |
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.
we already have Object storage
nav, why add another one ??
"id": "storage-object", | ||
"insertAfter": "storage-overview", | ||
"section": "storage", | ||
"name": "%plugin__odf-console~Obejct Storage%", |
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.
as per current implementation we only show it once rgw/mcg is deployed. do we now need to show always ??
"insertAfter": "storage-overview", | ||
"section": "storage", | ||
"name": "%plugin__odf-console~Obejct Storage%", | ||
"href": "/odf/mcg" |
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.
"href": "/odf/mcg" | |
"href": "/odf/object-storage" |
we have rgw as well, /mcg
seems incorrect...
// eslint-disable-next-line no-console | ||
console.log('Redirecting to ', installationFlow); |
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.
// eslint-disable-next-line no-console | |
console.log('Redirecting to ', installationFlow); |
> | ||
<CardHeader | ||
selectableActions={{ | ||
onClickAction: redirectTo('storage-cluster'), |
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.
onClickAction: redirectTo('storage-cluster'), | |
onClickAction: redirectTo(StartingPoint.STORAGE_CLUSTER), |
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.
similarly at other places...
type DataFoundationInitialCreateModalProps = { | ||
closeModal: () => void; | ||
}; | ||
|
||
export const StorageClusterCreateModal: React.FC< | ||
DataFoundationInitialCreateModalProps |
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.
type DataFoundationInitialCreateModalProps = { | |
closeModal: () => void; | |
}; | |
export const StorageClusterCreateModal: React.FC< | |
DataFoundationInitialCreateModalProps | |
type StorageClusterCreateModalProps = { | |
closeModal: () => void; | |
}; | |
export const StorageClusterCreateModal: React.FC< | |
StorageClusterCreateModalProps |
<ConfigureDFSelections closeModal={closeModal} /> | ||
</Modal> | ||
); | ||
}; |
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.
File name is EmptyStateCreateModal.tsx
, but there is no component with such name. I think it's good to either rename the file to StorageClusterCreateModal.tsx
or rename component to EmptyStateCreateModal
.
EmptyStateIcon, | ||
} from '@patternfly/react-core'; | ||
import { StorageDomainIcon } from '@patternfly/react-icons'; | ||
import { StorageClusterCreateModal } from '../../../modals/ConfigureDF/EmptyStateCreateModal'; |
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.
nits:
import { StorageClusterCreateModal } from '../../../modals/ConfigureDF/EmptyStateCreateModal'; | |
import { StorageClusterCreateModal } from '@odf/core/modals/ConfigureDF/EmptyStateCreateModal'; |
import { StorageClusterCreateModal } from '../../../modals/ConfigureDF/EmptyStateCreateModal'; | ||
|
||
const getPageTitle = (path: string, t: TFunction) => { | ||
if (path.includes('storage-cluster')) { |
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.
if (path.includes('storage-cluster')) { | |
if (path.includes(StartingPoint.StorageCluster)) { |
const pathParts = pathname.split('/'); | ||
const lastPart = pathParts.pop(); | ||
switch (lastPart) { | ||
case 'storage-cluster': |
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.
plz use StartingPoint
enum...
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.
same for other cases...
<Button | ||
variant="link" | ||
onClick={() => { | ||
return null; | ||
}} | ||
> | ||
{t('Documentation link')} | ||
</Button> |
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.
nits: better to not show it at all until we have doc link (if we miss adding link, we will get unnecessary bugs later)... and add a ToDo comment instead...
<Button | |
variant="link" | |
onClick={() => { | |
return null; | |
}} | |
> | |
{t('Documentation link')} | |
</Button> |
"id": "storage-object", | ||
"insertAfter": "storage-overview", | ||
"section": "storage", | ||
"name": "%plugin__odf-console~Obejct Storage%", |
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.
"name": "%plugin__odf-console~Obejct Storage%", | |
"name": "%plugin__odf-console~Object Storage%", |
No description provided.