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

CONSOLE-2361: Update react-catalog-view-extension to version that does not require PatternFly 3 #9419

Merged
merged 1 commit into from Jul 15, 2021
Merged

CONSOLE-2361: Update react-catalog-view-extension to version that does not require PatternFly 3 #9419

merged 1 commit into from Jul 15, 2021

Conversation

rhamilto
Copy link
Member

@rhamilto rhamilto commented Jul 6, 2021 •
edited

This PR should result in no significant changes in the rendering of the catalog, but migrate us to a version of react-catalog-view-extension that does not include Bootstrap/PatternFly 3 as a dependency.

Notes:

  • I observed one very subtle difference in the distance between the checkbox and the checkbox label in FilterSidePanelCategoryItem, but the difference is negligible.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2021
@openshift-ci openshift-ci bot added component/core Related to console core functionality approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 6, 2021
@rhamilto rhamilto changed the title [WIP] Update react-catalog-view-extension to version that does not require PatternFly 3 [WIP] CONSOLE-2361: Update react-catalog-view-extension to version that does not require PatternFly 3 Jul 8, 2021
@rhamilto rhamilto changed the title [WIP] CONSOLE-2361: Update react-catalog-view-extension to version that does not require PatternFly 3 CONSOLE-2361: Update react-catalog-view-extension to version that does not require PatternFly 3 Jul 8, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2021
@rhamilto
Copy link
Member Author

rhamilto commented Jul 8, 2021

/hold for approvals
/assign @yapei

@yapei We just need a regression test to make sure there are no visual problems updating this dependency.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2021
dtaylor113
dtaylor113 approved these changes Jul 9, 2021
View reviewed changes
Copy link
Contributor

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2021
@rhamilto
Copy link
Member Author

rhamilto commented Jul 9, 2021

/hold

See patternfly/patternfly#4155 (comment)

I think we should wait for the revert, and then the override I added won't be necessary.

@yanpzhan
Copy link
Contributor

Tested the pr, did some regression test on operatorhub page and catalog page. Didn't find issue.
@rhamilto if QE needs test again after patternfly issue reverts and your fix for it is removed?

@rhamilto
Copy link
Member Author

Tested the pr, did some regression test on operatorhub page and catalog page. Didn't find issue.
@rhamilto if QE needs test again after patternfly issue reverts and your fix for it is removed?

@yanpzhan, I don't think a second QE test is necessary since the gist of this PR is to migrate to a version of react-catalog-view-extension that does not include Bootstrap/PatternFly 3 as a dependency, which will not require retesting.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2021
@rhamilto rhamilto mentioned this pull request Jul 13, 2021
Merged
rhamilto
rhamilto commented Jul 13, 2021
View reviewed changes
frontend/public/style/_overrides.scss
@@ -1,5 +1,4 @@
// Use this file to override styles from 3rd party dependencies
$pf-4-nav-bar-height: 76px; // Height of the PatternFly 4 masthead
Copy link
Member Author

Choose a reason for hiding this comment

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

This really should have gone with #9452 as it is orphaned, but @spadgett already tagged that and I don't want him to have to retag it.

@yanpzhan
Copy link
Contributor

/label qe-approved

@openshift-ci openshift-ci bot added qe-approved Signifies that QE has signed off on this PR needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 14, 2021
@rhamilto rhamilto force-pushed the catalog-extension branch 2 times, most recently from 53bf1ca to 1493dea Compare July 14, 2021 14:19
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2021
@rhamilto
Copy link
Member Author

/hold cancel

The PatternFly regression was reverted, and this PR now makes use of the PatternFly version without the regression.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2021
@rhamilto
Copy link
Member Author

/retest

@spadgett spadgett added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/dependency-change Categorizes issue or PR as related to changing dependencies labels Jul 14, 2021
@spadgett
Copy link
Member

It looks like the catalog extension is getting pulled into the main vendor bundle causing the analyze job failure.

@spadgett
Copy link
Member

Or maybe just Patternfly is a little bigger, and we went over the max.

@rhamilto
Copy link
Member Author

rhamilto commented Jul 14, 2021 •
edited

Or maybe just Patternfly is a little bigger, and we went over the max.

I'm thinking this must be the case as I can't see a change that would result in a difference.

@spadgett spadgett mentioned this pull request Jul 14, 2021
Merged
@spadgett
Copy link
Member

See #9069 (comment). We added a new library, but didn't increase the vendor bundle limit enough IMO.

@spadgett
Copy link
Member

Let's increase the limit to something like 3.25MB for now and we can revisit whether we want to lazy load the quick starts library separately.

cc @jschuler

@rhamilto
Copy link
Member Author

Let's increase the limit to something like 3.25MB for now and we can revisit whether we want to lazy load the quick starts library separately.

Done.

spadgett
spadgett approved these changes Jul 14, 2021
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtaylor113, rhamilto, spadgett

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit c3bbe97 into openshift:master Jul 15, 2021
@rhamilto rhamilto deleted the catalog-extension branch July 15, 2021 11:52
@sg00dwin sg00dwin mentioned this pull request Jul 15, 2021
Merged
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

@dtaylor113 dtaylor113 dtaylor113 approved these changes

@sg00dwin sg00dwin Awaiting requested review from sg00dwin

Assignees

@spadgett spadgett

@yapei yapei

@dtaylor113 dtaylor113

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/dependency-change Categorizes issue or PR as related to changing dependencies lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
v4.9
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rhamilto @yanpzhan @spadgett @dtaylor113 @yapei @openshift-merge-robot