Skip to content

fix(nav): Fix bug where previous was expanding on select #1479

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
Merged
tlabaj merged 1 commit into from
Mar 4, 2019
Merged

fix(nav): Fix bug where previous was expanding on select #1479

tlabaj merged 1 commit into from
Mar 4, 2019

Conversation

@ibolton336
Copy link
Member

@ibolton336 ibolton336 commented Mar 1, 2019

What:
#1016

Additional issues:

@ibolton336 ibolton336 requested a review from priley86 March 1, 2019 19:59
@patternfly-build
Copy link
Collaborator

patternfly-build commented Mar 1, 2019

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

@codecov-io
Copy link

codecov-io commented Mar 1, 2019 •
edited
Loading

Codecov Report

Merging #1479 into master will not change coverage.
The diff coverage is 20%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1479   +/-   ##
=======================================
  Coverage   83.41%   83.41%           
=======================================
  Files         543      543           
  Lines        5619     5619           
  Branches       12       12           
=======================================
  Hits         4687     4687           
  Misses        930      930           
  Partials        2        2
Flag Coverage Δ
#patternfly3 84.94% <ø> (ø) ⬆️
#patternfly4 80.6% <20%> (ø) ⬆️
Impacted Files Coverage Δ
.../patternfly-4/react-core/src/components/Nav/Nav.js 80% <ø> (ø) ⬆️
...rnfly-4/react-core/src/components/Nav/NavToggle.js 41.66% <0%> (ø) ⬆️
...y-4/react-core/src/components/Nav/NavExpandable.js 81.81% <100%> (ø) ⬆️

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 61df512...f92a086. Read the comment docs.

priley86
priley86 reviewed Mar 1, 2019
View reviewed changes
packages/patternfly-4/react-core/src/components/Nav/NavToggle.js
componentWillReceiveProps(nextProps) {
if (nextProps.defaultValue !== this.state.value) {
this.setState({ value: nextProps.defaultValue });
componentDidUpdate(prevProps) {
Copy link
Member

@priley86 priley86 Mar 1, 2019

Choose a reason for hiding this comment

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

👍

@priley86
Copy link
Member

priley86 commented Mar 1, 2019

LGTM - 🐛 🔨

after

priley86
priley86 previously approved these changes Mar 1, 2019
View reviewed changes
priley86
priley86 previously approved these changes Mar 1, 2019
View reviewed changes
@priley86 priley86 mentioned this pull request Mar 1, 2019
Merged
jschuler
jschuler reviewed Mar 1, 2019
View reviewed changes
tlabaj
tlabaj approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@tlabaj tlabaj merged commit f0863a0 into patternfly:master Mar 4, 2019
@rhamilto
Copy link
Member

rhamilto commented Mar 4, 2019

This fix introduces some new bugs:

  1. Upon app load, active sections are not expanded.

yhnpvoa2v6

  1. Previously expanded items that have been collapsed items are (once again) not re-expanded.

ixkroplivy

cc: @spadgett, @janwright73, @priley86

@spadgett
Copy link

spadgett commented Mar 4, 2019

It seems like the underlying problem is that we're tracking the same value using both props and state, which is generally considered an anti-pattern. Would it be simpler just to let the outer component control expanded state and add a callback for when the nav section is activated? Then the expanded prop is the single source of truth.

It puts more work on the outer component, but I don't see a good way around that as long as the expanded state can change based on things that happen outside the nav (such as the user clicking on a link on a details page).

@priley86
Copy link
Member

priley86 commented Mar 5, 2019

this should mostly be resolved now. One step closer here...
openshift/console#1238 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tlabaj tlabaj tlabaj approved these changes

@kmcfaul kmcfaul kmcfaul approved these changes

@karelhala karelhala Awaiting requested review from karelhala

@dgutride dgutride Awaiting requested review from dgutride

+2 more reviewers

@jschuler jschuler jschuler left review comments

@priley86 priley86 priley86 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ibolton336 @patternfly-build @codecov-io @priley86 @rhamilto @spadgett @jschuler @tlabaj @kmcfaul