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.

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

refactor(HMS-4079): upgrade to PatternFly 5 #62

Merged
merged 8 commits into from
May 15, 2024

Conversation

pvoborni
Copy link
Contributor

This PR brings update of the whole UI to PatternFly 5. Replacement of deprecated components will be done separately.

Better to check by individual commits and also see commit messages there.

The update of PatternFly required also update of various other libraries so that the versions works across dependencies. In general I tried to bring the latest versions but in react I used the same as in front end components.

@app-sre-bot
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@avisiedo avisiedo left a comment

Choose a reason for hiding this comment

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

Thanks for the change 👍

Checked locally and the frontend run correctly.

only an additional change:

  • Update .rhcicd/build_deploy.sh to update export NODE_BUILD_VERSION=18 to override the node version used for cicd platform.
  • Running make container-build it was got a situation with the number of opened files; at scripts/mk/container.mk adding the option --ulimit nofile=65535:65535 when running the container engine for container-buildrule fixed that scenario during the collab session (thanks for the help to check the right value 👍).

@avisiedo
Copy link
Contributor

LGTM

@avisiedo avisiedo self-requested a review May 15, 2024 07:20
@avisiedo
Copy link
Contributor

@pvoborni sorry 🙏 I didn't realize about the message that it needs rebase.

@frasertweedale
Copy link
Contributor

Sorry for that conflict, it is due to #59 which I merged a few minutes ago.

pvoborni added 8 commits May 15, 2024 17:46
All changes in this commit were created by running:

```
npx @patternfly/pf-codemods@latest src --fix
```

It used version `@patternfly/[email protected]`

Signed-off-by: Petr Vobornik <[email protected]>
Upgrade PatternFly libraries to major version 5. This caused also
cascade of version bumps of other dependencies in order to work.

Signed-off-by: Petr Vobornik <[email protected]>
Fixes prettier issues after the automatic code conversion by codemods.

Signed-off-by: Petr Vobornik <[email protected]>
`fec` build had issues with finding `PageSectionTypes`. To make
it working, it would probably be needed to update `COMPONENTS_CACHE`
in `tsc-transform-imports`. It was easier to replace with string.

`SampleComponent` was breaking build its props definition needed to
be more explicit.

Upgrade of `eslint-config-redhat-cloud-services` or related package
caused that eslint failed in most PF imports with
`rulesdir/forbid-pf-relative-imports` rule. Thus this rule was disabled
to keep the current change small.

hotReload defintion in fec config was changed as FEC warned that previous
way was deprecated.

Signed-off-by: Petr Vobornik <[email protected]>
This commit is result of run:

```
npx @patternfly/class-name-updater src --fix
```

That is migrationg old PF class names to v5 variants.

Signed-off-by: Petr Vobornik <[email protected]>
With upgrade to PF5, the size of content of idmsvc-frontend was
heigher than the space offered. I.e. scrollbars were displayed in
default page even when there was not enough content or in wizard,
the row with bottom buttons was not visible and required scrolling.

`<Page>` component is typically a high-level container which e.g.
contains also `Masthead` and `Navigation` but this is part of "chrome".
So in this app, all "page" is actually just the "page content" and
thus "PageGroup" is the suitable component.

This change fixes the size/padding issues so scrollbars are not
displayed when not needed.

Signed-off-by: Petr Vobornik <[email protected]>
1. issue in DomaiList where react complained that items in iterator/map don't have
   unique `key`.
2. two instances of:

```
Th: Table headers must have an accessible name. If the Th is intended to be
visually empty, pass in screenReaderText. If the Th contains only non-text,
interactive content such as a checkbox or expand toggle, pass in an aria-label
```

Signed-off-by: Petr Vobornik <[email protected]>
As builds on 16 produces warnings:
```
npm install
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '@testing-library/[email protected]',
npm WARN EBADENGINE   required: { node: '>=18' },
npm WARN EBADENGINE   current: { node: 'v16.20.2', npm: '8.19.4' }
npm WARN EBADENGINE }
```

Signed-off-by: Petr Vobornik <[email protected]>
@avisiedo
Copy link
Contributor

LGTM merging

@avisiedo avisiedo merged commit 0cbdf84 into podengo-project:main May 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants