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

✨ Migration to PF V5 #1078

Merged
merged 32 commits into from
Jul 10, 2023
Merged

✨ Migration to PF V5 #1078

merged 32 commits into from
Jul 10, 2023

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Jun 29, 2023

The initial driver behind the need for PatternFly Version 5 is for the datePicker component which get bug fixes.
But we'll need to migration to V5 anyway sooner than later.

Some components (Table, Select, Dropdown, Wizard) have a V4 version maintained in @patternfly/react-core/deprecated allowing for a smooth transition which can help us make sure there is no UX changes.
Once migrated we can progressively move to the newer components.

This PR addresses the 150+ errors initially triggered by porting to pre-release V5.

TODO (more likely in following PRs):

Tests :

  • Skipped 4 test in proxy-form-tests: this is a timing issue as the data is still loading
  • Skip 1 test in application-form.test due to timeout.
  • Skipped SidebarAppTest and DefaultLayout.test because every run renders arial-labelledby with different values

@gildub gildub requested a review from mturley June 29, 2023 20:54
@gildub gildub self-assigned this Jun 29, 2023
@gildub gildub changed the title Migration to PF V5 [WIP] Migration to PF V5 Jun 29, 2023
@gildub gildub changed the title [WIP] Migration to PF V5 [WIP] ✨ Migration to PF V5 Jun 29, 2023
@ibolton336
Copy link
Member

@gildub @mturley Didn't you say there was a way to pick up a single beta component rather than updating the entire project just yet?

@mturley
Copy link
Collaborator

mturley commented Jun 30, 2023

@ibolton336 for certain components that had major changes, they released them early under @patternfly/react-core/next. But I hadn't realized that was only for the new Dropdown, Select and Wizard. https://github.com/patternfly/patternfly-react/tree/v4/packages/react-core/src/next/components

@gildub gildub changed the title [WIP] ✨ Migration to PF V5 ✨ Migration to PF V5 Jul 6, 2023
gildub and others added 22 commits July 6, 2023 21:20
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Mike Turley <mike.turley@alum.cs.umass.edu>
Signed-off-by: Mike Turley <mike.turley@alum.cs.umass.edu>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Mike Turley <mike.turley@alum.cs.umass.edu>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Patch coverage: 31.68% and project coverage change: -2.60 ⚠️

Comparison is base (a8e6de5) 46.70% compared to head (0e5a20c) 44.11%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1078      +/-   ##
==========================================
- Coverage   46.70%   44.11%   -2.60%     
==========================================
  Files         177      177              
  Lines        4462     4477      +15     
  Branches     1001      997       -4     
==========================================
- Hits         2084     1975     -109     
- Misses       2364     2491     +127     
+ Partials       14       11       -3     
Flag Coverage Δ
unitests 44.11% <31.68%> (-2.60%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/src/app/common/CustomRules/useRuleFiles.tsx 6.71% <0.00%> (ø)
client/src/app/common/KeyDisplayToggle.tsx 100.00% <ø> (ø)
...ent/src/app/layout/AppAboutModal/AppAboutModal.tsx 83.33% <ø> (ø)
...tions/analysis-wizard/components/upload-binary.tsx 13.51% <0.00%> (ø)
...p/pages/applications/analysis-wizard/set-scope.tsx 26.47% <0.00%> (ø)
...s/components/application-form/application-form.tsx 55.98% <0.00%> (-0.55%) ⬇️
...ntities/components/identity-form/identity-form.tsx 56.45% <0.00%> (-13.31%) ⬇️
client/src/app/pages/proxies/proxies.tsx 100.00% <ø> (ø)
client/src/app/pages/proxies/proxy-form.tsx 34.45% <0.00%> (-27.94%) ⬇️
...d/components/FilterToolbar/SearchFilterControl.tsx 20.00% <0.00%> (-1.06%) ⬇️
... and 34 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 31 to 49
.pf-c-icon .pf-m-default {
color: var(--pf-c-icon--Color);
.pf-v5-c-icon .pf-v5-m-default {
color: var(--pf-v5-c-icon--Color);
}

.pf-c-icon.pf-m-info {
color: var(--pf-c-icon--m-info--Color);
.pf-v5-c-icon.pf-v5-m-info {
color: var(--pf-v5-c-icon--m-info--Color);
}

.pf-c-icon .pf-m-success {
color: var(--pf-c-icon--m-success--Color);
.pf-v5-c-icon .pf-v5-m-success {
color: var(--pf-v5-c-icon--m-success--Color);
}

.pf-c-icon .pf-m-warning {
color: var(--pf-c-icon--m-warning--Color);
.pf-v5-c-icon .pf-v5-m-warning {
color: var(--pf-v5-c-icon--m-warning--Color);
}

.pf-c-icon .pf-m-danger {
color: var(--pf-c-icon--m-danger--Color);
.pf-v5-c-icon .pf-v5-m-danger {
color: var(--pf-v5-c-icon--m-danger--Color);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised that this CSS was ever necessary. I wonder if we just remove all the pf-v5-c-icon stuff here if icons work as expected?

@gildub it looks like this was added in https://github.com/konveyor/tackle2-ui/pull/300/files#diff-acb6efd446c502574b8cfc71c2c9fda16948c4811f0040ed7efcbfd1ebdee22f, do you remember why it was needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for point it that.
I'll check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mturley, Effectively those classes seems unnecessary, I removed them. I can't remember why I added them in the first place.

Copy link
Collaborator

@mturley mturley left a comment

Choose a reason for hiding this comment

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

I haven't found any more glaring behavior regressions yet. I think this is at a point where I would be comfortable merging it as long as we make a followup effort over the next few days to thoroughly compare the before/after visually and open issues for each visual/style regression, then prioritize getting those fixed before long.

Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
@gildub gildub merged commit f422fb4 into konveyor:main Jul 10, 2023
@gildub gildub deleted the mta-706 branch July 10, 2023 12:50
gildub added a commit that referenced this pull request Jul 11, 2023
#1078 follow-up to restore
personae select look and feel.
mturley added a commit that referenced this pull request Jul 11, 2023
…#1104)

I missed this when reviewing #1078. cc @gildub

I noticed that when clicking an affected file in the drawer on the
Issues pages to open the incident details modal, the last tab ("All
incidents" table) was split out into its own tab bar and rendered below
the contents of the selected tab. This is because of this change:
https://github.com/konveyor/tackle2-ui/pull/1078/files#diff-e40974378de11be89b5b1ca02db26b8fed80482f468154ebb84807af7f0e7273L125-R147
(link takes a while to scroll the diff after loading)

When I reversed the change, I saw the reason for it: the new Tabs
component doesn't like to have its children expressed as a combination
of an array and extra nodes (like we had been doing when mapping over
the first 5 incidents and conditionally including a 6th tab). It gave
this error:

![Screenshot 2023-07-10 at 3 38 00
PM](https://github.com/konveyor/tackle2-ui/assets/811963/6d10427c-15c5-47bb-973d-2110b2f2b48f)

It appears to be due to this type change in PF:
patternfly/patternfly-react#8217

The solution was to make sure the children of `<Tabs>` is always a
single array of `<Tab>` elements.

---------

Signed-off-by: Mike Turley <mike.turley@alum.cs.umass.edu>
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.

3 participants