Bug 1722876: Fix Infinite Re-Render Bug in Operand List Sort #1863
Conversation
openshift-ci-robot
added
the
size/L
openshift-ci-robot
added
the
approved
| @@ -116,7 +116,7 @@ export const navFactory: NavFactory = { | |||
| }), | |||
| }; | |||
|
|
|||
| export const NavBar: React.SFC<NavBarProps> = ({pages, basePath, hideDivider}) => { | |||
| export const NavBar = withRouter<NavBarProps>(({pages, basePath, hideDivider}) => { | |||
There was a problem hiding this comment.
This is actually a better solution to ensure NavBar re-renders when the route changes. Before it was depending on HorizontalNav to be re-rendered, but using withRouter ensures we capture route changes.
| NavBar.displayName = 'NavBar'; | ||
|
|
||
| export class HorizontalNav extends React.PureComponent<HorizontalNavProps> { |
There was a problem hiding this comment.
Converted to functional component with useMemo() to prevent unmounting child <Route> components.
| import ConnectedEmptyStateComponent from '../EmptyState'; | ||
| import { getStoreTypedComponent } from '../../test/test-utils'; | ||
|
|
||
| describe('EmptyState', () => { |
There was a problem hiding this comment.
Doesn't really test anything, and fails when upgrading to newer react-redux version.
spadgett
changed the title
|
@alecmerdler: This pull request references a valid Bugzilla bug. The bug has been moved to the POST state. In response to this:
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. |
openshift-ci-robot
added
the
bugzilla/valid-bug
alecmerdler
force-pushed
the
bugzilla-1722876
branch
from
f7804b6
to
14a3d84
Compare
alecmerdler
force-pushed
the
bugzilla-1722876
branch
from
14a3d84
to
54aedcd
Compare
|
/lgtm |
|
Looks like integration test login is broken with this change |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/hold ...to prevent automatic retesting. The login tests are consistently failing. |
openshift-ci-robot
added
the
do-not-merge/hold
alecmerdler
force-pushed
the
bugzilla-1722876
branch
from
54aedcd
to
6981ce6
Compare
openshift-ci-robot
added
size/XL
| @@ -24,12 +24,6 @@ import { | |||
| PageSectionVariants, | |||
| } from '@patternfly/react-core'; | |||
|
|
|||
| // React Router's proptypes are incorrect. See https://github.com/ReactTraining/react-router/pull/5393 | |||
| (Route as any).propTypes.path = PropTypes.oneOfType([ | |||
There was a problem hiding this comment.
This was causing problems with yarn run build. Issue appears to be fixed with newer version of library.
|
/hold cancel |
openshift-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alecmerdler, 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 |
|
We got past login this time /retest |
|
Tests are passing locally for me, so they must be CI flakes. |
Description
Use
React.memo()andReact.useCallback()to preventHorizontalNavfrom re-rendering unnecessarily, which causes the detail page route components to be mounted/unmounted when usingprops.pagesFor().Upgrade
react-reduxto work withReact.memo()Upgrade
react-router,react-router-domso that they work withReact.memo()Upgrade
react-helmetbecause of another call stack issue.Also renames components, types, and other symbols from using
ClusterServiceVersionResourcetoOperand.Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1722876