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

Bug 1854655: Bump to latest @patternfly/patternfly release (4.23.1) #5987

Merged
merged 1 commit into from Jul 14, 2020

Conversation

dtaylor113
Copy link
Contributor

  • Updated @patternfly/react-core to 4.31.1, and @patternfly/react-topology to 4.4.25
  • Removed tippyProps in Tooltips
  • Updated remaining @patternfly packages to latest versions
  • Smoke test revealed no visual or behavioral regressions

@dtaylor113
Copy link
Contributor Author

dtaylor113 commented Jul 14, 2020

Seeing failures for unit tests which reference /console/frontend/node_modules/@popperjs:

FAIL __tests__/components/routes/create-route.spec.tsx
  ● Test suite failed to run
    /go/src/github.com/openshift/console/frontend/node_modules/@popperjs/core/lib/utils/getOppositePlacement.js:7
    export default function getOppositePlacement(placement) {
    ^^^^^^
    
    SyntaxError: Unexpected token export
      
      at new Script (vm.js:83:7)
      at Object.<anonymous> (node_modules/@patternfly/react-core/dist/js/helpers/Popper/Popper.js:8:56)

UPDATE: fixed, had to add @popperjs to jest transformIgnorePatterns

@@ -37,7 +36,7 @@ type PopoverStatusProps = {
title?: string;
hideHeader?: boolean;
isVisible?: boolean;
shouldClose?: (tip: TippyInstance) => void;
shouldClose?: (hideFnc: any) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Can you improve the types here?

nit: I'd spell it out to make it more clear: hideFunction (I'm assuming fnc means function)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, changed to hideFunction. Can you pls elaborate on how to improve types? Using what PF-React has for Popover

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the code it seems PF didn't update their types.

shouldClose?: (hideFnc: () => void) => void;

@openshift openshift deleted a comment from openshift-ci-robot Jul 14, 2020
@dtaylor113
Copy link
Contributor Author

PatternFly team working on a PR ( patternfly/patternfly-react#4543) which will restore tippyProps to avoid breaking changes. They are hoping to get the PR merged and part of this release.

@dtaylor113
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2020
@christianvogt
Copy link
Contributor

@dtaylor113 do we really need to wait for that PR? It doesn't affect us.

@jschuler
Copy link
Contributor

I'm restoring tippyProps in that PR but it is really just a bandaid so that the build won't fail if react-core dependency is updated to latest. It has been deprecated. It'll be better to remove it as well as imported types from tippy.js. Please let me know if you have any other concerns/issues :)

@dtaylor113
Copy link
Contributor Author

dtaylor113 commented Jul 14, 2020

@dtaylor113 do we really need to wait for that PR? It doesn't affect us.

@christianvogt, I guess not as this PR makes the required changes to work with the latest Popover.

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2020
@dtaylor113
Copy link
Contributor Author

/retest

@openshift openshift deleted a comment from openshift-ci-robot Jul 14, 2020
@christianvogt
Copy link
Contributor

Ran PR and roamed the app. Tested topology tooltips.
/lgtm
/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, dtaylor113

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-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 14, 2020
@openshift-merge-robot openshift-merge-robot merged commit dccfe8f into openshift:master Jul 14, 2020
@spadgett spadgett changed the title Bump to latest @patternfly/patternfly release (4.23.1) Bug 1854655: Bump to latest @patternfly/patternfly release (4.23.1) Jul 15, 2020
@spadgett
Copy link
Member

This indirectly fixes https://bugzilla.redhat.com/show_bug.cgi?id=1854655

@openshift-ci-robot
Copy link
Contributor

@dtaylor113: All pull requests linked via external trackers have merged: . Bugzilla bug 1854655 has been moved to the MODIFIED state.

In response to this:

Bug 1854655: Bump to latest @patternfly/patternfly release (4.23.1)

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.

@spadgett spadgett added this to the v4.6 milestone Jul 16, 2020
@spadgett spadgett added the kind/dependency-change Categorizes issue or PR as related to changing dependencies label Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared kind/dependency-change Categorizes issue or PR as related to changing dependencies lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants