fix(navExpand): remove defaultExpand in favor of just using isExpanded #1490

ibolton336 · 2019-03-04T20:18:47Z

What:
#1016

Additional issues:

patternfly-build · 2019-03-04T20:31:58Z

PatternFly-React preview: https://1490-pr-patternfly-react-patternfly.surge.sh

codecov-io · 2019-03-04T20:35:04Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ca79c1e). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #1490   +/-   ##
========================================
  Coverage          ?   83.4%           
========================================
  Files             ?     545           
  Lines             ?    5653           
  Branches          ?      12           
========================================
  Hits              ?    4715           
  Misses            ?     936           
  Partials          ?       2

Flag	Coverage Δ
#patternfly3	`84.89% <ø> (?)`
#patternfly4	`80.68% <ø> (?)`

Impacted Files	Coverage Δ
...rnfly-4/react-core/src/components/Nav/NavToggle.js	`41.66% <ø> (ø)`
...y-4/react-core/src/components/Nav/NavExpandable.js	`81.81% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca79c1e...aefa59b. Read the comment docs.

codecov-io · 2019-03-04T20:35:04Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ca79c1e). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #1490   +/-   ##
========================================
  Coverage          ?   83.4%           
========================================
  Files             ?     545           
  Lines             ?    5653           
  Branches          ?      12           
========================================
  Hits              ?    4715           
  Misses            ?     936           
  Partials          ?       2

Flag	Coverage Δ
#patternfly3	`84.89% <ø> (?)`
#patternfly4	`80.68% <ø> (?)`

Impacted Files	Coverage Δ
...rnfly-4/react-core/src/components/Nav/NavToggle.js	`41.66% <ø> (ø)`
...y-4/react-core/src/components/Nav/NavExpandable.js	`81.81% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca79c1e...aefa59b. Read the comment docs.

codecov-io · 2019-03-04T20:35:05Z

Codecov Report

Merging #1490 into master will decrease coverage by 0.01%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master    #1490      +/-   ##
==========================================
- Coverage    83.4%   83.39%   -0.02%     
==========================================
  Files         545      545              
  Lines        5653     5660       +7     
  Branches       12       12              
==========================================
+ Hits         4715     4720       +5     
- Misses        936      938       +2     
  Partials        2        2

Flag	Coverage Δ
#patternfly3	`84.89% <ø> (ø)`	⬆️
#patternfly4	`80.65% <44.44%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
...rnfly-4/react-core/src/components/Nav/NavToggle.js	`50% <0%> (+8.33%)`	⬆️
...y-4/react-core/src/components/Nav/NavExpandable.js	`75% <66.66%> (-6.82%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86824d9...ebab3d9. Read the comment docs.

tlabaj

LGTM

priley86 · 2019-03-04T20:41:13Z

this seems to have the desired effect since OpenShift is currently only using the isExpanded prop to drive this. I'm not 100% positive of the OpenShift UX rules on this, so please correct me if I'm wrong @rhamilto .

Opening a link with "Home" nav section expanded will correctly now open the "Workloads" section on route change:

Opening a link with the "Home" section subsequently closed will correctly maintain its state after route change:

Does this resolve @rhamilto ?

priley86 · 2019-03-04T20:44:39Z

on hard refresh, it seems to correctly expand the current nav for the currently matched route (since a child nav item is active):

kmcfaul

LGTM

priley86 · 2019-03-04T21:20:13Z

we are still resolving one other edge case... the isExpanded prop is not respected after nav toggle. This is almost there...

priley86 · 2019-03-05T14:43:06Z

packages/patternfly-4/react-core/src/components/Nav/NavExpandable.js

+    this.setState({ expandedState: this.props.isExpanded });
+  }
+  componentDidUpdate(prevProps) {
+    if (this.props.isExpanded !== prevProps.isExpanded) {


should this be:

componentDidUpdate() { if (this.props.isExpanded !== this.state.isExpanded) {

?

sorry:

if (this.props.isExpanded !== prevProps.isExpanded || this.props.isExpanded !== this.state.expandedState)

ok, made a few changes to address this. If the consumer is setting expandState externally, we should allow them to hook into this, otherwise they could be become out of sync. My changes here would allow the consumer to override the expand/collapse state at either the top level (Nav and the onToggle), or at a more granular level, the NavExpandable and the onExpand callback). They would then be able to hook into either (or neither if desired). See if this works on your side...
priley86@ee30e20

downstream explanation/relevant changes...
openshift/console#1238 (comment)

tlabaj

LGTM

rhamilto · 2019-03-05T21:06:40Z

So close. Console warning needs fixing (which @priley86 is doing).

Warning: Unknown event handler property `onExpand`. It will be ignored.
    in li (created by NavToggle)
    in NavToggle
    in NavExpandable (created by NavSection)
    in NavSection (created by Connect(NavSection))
    in Connect(NavSection) (created by Navigation)
    in ul (created by NavList)
    in NavList (created by Navigation)
    in nav (created by Nav)
    in Nav (created by Navigation)
    in div (created by PageSidebar)
    in PageSidebar (created by Navigation)
    in Navigation (created by App)
    in div (created by Page)
    in Page (created by App)
    in App (created by Route)
    in Route
    in Router
    in Provider

priley86 · 2019-03-05T21:25:45Z

thanks @rhamilto - #1504 is up... thanks for your patience 👏

ibolton336 requested review from priley86 and tlabaj March 4, 2019 20:18

tlabaj previously approved these changes Mar 4, 2019

View reviewed changes

kmcfaul previously approved these changes Mar 4, 2019

View reviewed changes

ibolton336 added the Do Not Merge label Mar 4, 2019

ibolton336 dismissed stale reviews from kmcfaul and tlabaj via 1ec3296 March 5, 2019 02:31

ibolton336 force-pushed the nav-fix-3 branch from 1ec3296 to 0084e10 Compare March 5, 2019 02:34

ibolton336 added bug 🐛 and removed Do Not Merge labels Mar 5, 2019

priley86 reviewed Mar 5, 2019

View reviewed changes

ibolton336 force-pushed the nav-fix-3 branch 5 times, most recently from 497ef42 to 5e3fb34 Compare March 5, 2019 16:51

priley86 previously approved these changes Mar 5, 2019

View reviewed changes

priley86 mentioned this pull request Mar 5, 2019

Bump patternfly* to latest versions to pick up bug fixes openshift/console#1238

Merged

ibolton336 dismissed priley86’s stale review via ebab3d9 March 5, 2019 18:26

ibolton336 force-pushed the nav-fix-3 branch from 5e3fb34 to ebab3d9 Compare March 5, 2019 18:26

fix(navExpand): remove defaultExpand in favor of just using isExpanded

90808de

ibolton336 force-pushed the nav-fix-3 branch from ebab3d9 to 90808de Compare March 5, 2019 18:48

priley86 approved these changes Mar 5, 2019

View reviewed changes

priley86 requested a review from redallen March 5, 2019 19:07

redallen merged commit 9aafabd into patternfly:master Mar 5, 2019

tlabaj reviewed Mar 5, 2019

View reviewed changes

priley86 mentioned this pull request Mar 5, 2019

fix(nav): fix React warning with onExpand prop #1504

Merged

fix(navExpand): remove defaultExpand in favor of just using isExpanded #1490

fix(navExpand): remove defaultExpand in favor of just using isExpanded #1490

Uh oh!

Conversation

ibolton336 commented Mar 4, 2019

Uh oh!

patternfly-build commented Mar 4, 2019

Uh oh!

codecov-io commented Mar 4, 2019

Codecov Report

Uh oh!

codecov-io commented Mar 4, 2019

Codecov Report

Uh oh!

codecov-io commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

priley86 commented Mar 4, 2019

Uh oh!

priley86 commented Mar 4, 2019

Uh oh!

kmcfaul left a comment

Choose a reason for hiding this comment

Uh oh!

priley86 commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priley86 Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

priley86 Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

priley86 Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

priley86 Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

rhamilto commented Mar 5, 2019

Uh oh!

priley86 commented Mar 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov-io commented Mar 4, 2019 •

edited

Loading

priley86 commented Mar 4, 2019 •

edited

Loading