Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Upgrade to Patternfly 5 #1107

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Upgrade to Patternfly 5 #1107

merged 1 commit into from
Mar 7, 2024

Conversation

bipuladh
Copy link
Contributor

Major changes:

  1. Packages related to Patternfly upgraded to v5.
  2. Changes to how form validation is done.
    ......

SanjalKatiyar
SanjalKatiyar reviewed Mar 6, 2024
View reviewed changes
packages/mco/components/create-dr-policy/select-cluster-list.tsx Outdated Show resolved Hide resolved
packages/mco/components/empty-state-page/empty-page.tsx Outdated Show resolved Hide resolved
packages/mco/components/mco-dashboard/disaster-recovery/cluster-app-card/common.tsx
Comment on lines +41 to +44
Select as SelectNext /* data-codemods */,
SelectOption as SelectOptionNext /* data-codemods */,
SelectList as SelectListNext /* data-codemods */,
SelectGroup as SelectGroupNext /* data-codemods */,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we remove /* data-codemods */ comments now ?

packages/mco/components/modals/app-data-policies-status/subscriptions/dr-status-table.tsx Outdated Show resolved Hide resolved
packages/mco/components/modals/app-manage-policies/policy-list-view.tsx
{t('No assigned data policy found')}
</Title>
<EmptyStateHeader
titleText={<>{t('No assigned data policy found')}</>}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
titleText={<>{t('No assigned data policy found')}</>}
titleText={t('No assigned data policy found')}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expects React.ReactNode

packages/shared/src/table/selectable-table.tsx Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
webpack.config.ts Outdated Show resolved Hide resolved
webpack.config.ts Outdated Show resolved Hide resolved
webpack.config.ts Outdated Show resolved Hide resolved
GowthamShanmugam
GowthamShanmugam reviewed Mar 6, 2024
View reviewed changes
packages/ocs/storage-class/sc-form.tsx
@@ -460,18 +462,14 @@ export const StorageClassEncryption: React.FC<ProvisionerProps> = ({
return (
<div className="ocs-storage-class__form">
<Form>
<FormGroup
fieldId="storage-class-encryption"
helperTextInvalid={t('This is a required field')}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring helperTextInvalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a comment to add it in future as bug fix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@bipuladh bipuladh changed the title [WIP] Upgrade to Patternfly 5 Upgrade to Patternfly 5 Mar 6, 2024
GowthamShanmugam
GowthamShanmugam reviewed Mar 6, 2024
View reviewed changes
packages/odf/components/kms-config/thales-config.tsx Outdated Show resolved Hide resolved
packages/odf/components/kms-config/thales-config.tsx Outdated Show resolved Hide resolved
packages/odf/components/kms-config/name-address-port.tsx Outdated Show resolved Hide resolved
packages/odf/components/kms-config/hpcs-config.tsx Outdated Show resolved Hide resolved
packages/odf/components/kms-config/hpcs-config.tsx Outdated Show resolved Hide resolved
packages/odf/components/kms-config/hpcs-config.tsx Outdated
<HelperText>
{isValid(kms.apiKey.valid) === ValidatedOptions.error && (
<HelperTextItem
variant={isValid(kms.apiKey.valid) ? 'default' : 'error'}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
variant={isValid(kms.apiKey.valid) ? 'default' : 'error'}
variant={isValid(kms.apiKey.valid) }

packages/odf/components/kms-config/hpcs-config.tsx Outdated Show resolved Hide resolved
packages/odf/components/kms-config/vault-auth-methods.tsx Outdated Show resolved Hide resolved
packages/odf/components/kms-config/vault-auth-methods.tsx Outdated Show resolved Hide resolved
packages/odf/components/kms-config/vault-config.tsx Outdated Show resolved Hide resolved
@bipuladh
Updates PF from v4 to v5
a613b62 
Updates @openshift-console sdk to 1.0.0

Signed-off-by: Bipul Adhikari <badhikar@redhat.com>
SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
analyzeTest.ts
@@ -1,7 +1,7 @@
import * as fs from 'fs';

const pluginName = process.env.PLUGIN;
const MAX_ASSET_SIZE = 3; //3 MiB
const MAX_ASSET_SIZE = 17; //17 MiB
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this intentional ? or were you testing something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I will dicuss with ocp team later to reduce bundle size

@bipuladh
Copy link
Contributor Author

bipuladh commented Mar 7, 2024

/retest

@bipuladh
Copy link
Contributor Author

bipuladh commented Mar 7, 2024

/test odf-console-e2e-aws

1 similar comment
@bipuladh
Copy link
Contributor Author

bipuladh commented Mar 7, 2024

/test odf-console-e2e-aws

@SanjalKatiyar
Copy link
Collaborator

/override ci/prow/odf-console-e2e-aws

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 7, 2024

@SanjalKatiyar: Overrode contexts on behalf of SanjalKatiyar: ci/prow/odf-console-e2e-aws

In response to this:

/override ci/prow/odf-console-e2e-aws

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.

@SanjalKatiyar
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 7, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, SanjalKatiyar

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:
  • OWNERS [SanjalKatiyar,bipuladh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@SanjalKatiyar
Copy link
Collaborator

/override ci/prow/odf-console-e2e-aws

need to merge this in order to send/test followup CI fixes on release side...

@openshift-merge-bot openshift-merge-bot bot merged commit 7bc97d8 into red-hat-storage:master Mar 7, 2024
5 checks passed
SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
packages/mco/components/modals/protected-applications/managed-applications-modal.tsx
@@ -80,7 +81,9 @@ const ManagedApplicationsModal: React.FC<CommonModalProps> = (props) => {
isInline
>
{t('Continue to Applications page')}
<ArrowRightIcon size={IconSize.sm} className="pf-u-ml-sm" />
<Icon size="sm">
<ArrowRightIcon className="pf-u-ml-sm" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are other references in this file too...

Suggested change
<ArrowRightIcon className="pf-u-ml-sm" />
<ArrowRightIcon className="pf-v5-u-ml-sm" />

SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
packages/mco/components/protected-applications/components.tsx
size={IconSize.sm}
className="pf-u-mr-sm"
/>
<OutlinedQuestionCircleIcon className="pf-u-mr-sm" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are other references in this file too...

Suggested change
<OutlinedQuestionCircleIcon className="pf-u-mr-sm" />
<OutlinedQuestionCircleIcon className="pf-v5-u-mr-sm" />

SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
packages/mco/components/protected-applications/list-page.tsx
@@ -202,6 +204,7 @@ const ProtectedAppsTableRow: React.FC<
<Td translate={null} isActionCell>
<ActionsColumn
items={getRowActions(t, launcher, navigate, application)}
translate={null}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file as well, contains references to PF4 classes...

SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
packages/shared/src/badges/Badge.scss
@@ -1,10 +1,10 @@
.ocs-preview-badge {
&.pf-c-label {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now we will have to override PF5 classes, I guess...

Suggested change
&.pf-c-label {
&.pf-v5-c-label {

SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
packages/shared/src/badges/Badge.scss
@@ -1,10 +1,10 @@
.ocs-preview-badge {
&.pf-c-label {
--pf-c-label--BackgroundColor: #d93f00;
--pf-c-label--BorderRadius: var(--pf-global--BorderRadius--sm);
--pf-c-label--BorderRadius: var(--pf-v5-global--BorderRadius--sm);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren't these variable names gonna get changed too ??

Suggested change
--pf-c-label--BorderRadius: var(--pf-v5-global--BorderRadius--sm);
--pf-v5-c-label--BorderRadius: var(--pf-v5-global--BorderRadius--sm);

SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
packages/shared/src/list-page/paginated-list-page.tsx
@@ -62,7 +62,6 @@ export const PaginatedsListPage: React.FC<PaginatedsListPageProps> = ({
</GridItem>
<GridItem md={4} sm={12}>
<Pagination
perPageComponent="button"
Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file contains ref to PF4 classes too...

SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
packages/shared/src/table/composable-table.tsx
@@ -63,11 +63,7 @@ export const ComposableTable: ComposableTableProps = <
NoDataEmptyMsg={noDataMsg}
skeleton={<div className="loading-skeleton--table pf-u-mt-lg" />}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PF4 refs in this file...

SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
packages/shared/src/table/selectable-table.tsx
@@ -101,7 +102,7 @@ export const SelectableTable: SelectableTableProps = <
data={sortedRows}
skeleton={<div className="loading-skeleton--table pf-u-mt-lg" />}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PF4 refs...

SanjalKatiyar
SanjalKatiyar reviewed Mar 7, 2024
View reviewed changes
packages/shared/src/table/table.tsx
@@ -103,7 +103,7 @@ const Table: React.FC<TableProps> = React.memo(
NoDataEmptyMsg={noDataMsg}
skeleton={<div className="loading-skeleton--table pf-u-mt-lg" />}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PF4...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GowthamShanmugam GowthamShanmugam GowthamShanmugam left review comments

@SanjalKatiyar SanjalKatiyar SanjalKatiyar left review comments

Assignees

@SanjalKatiyar SanjalKatiyar

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bipuladh @SanjalKatiyar @GowthamShanmugam @openshift-merge-robot