Skip to content

Fixed Pivot to show the correct PivotItem#8501

Merged
dzearing merged 1 commit into
microsoft:masterfrom
sebastianmattar:Fix-PivotItems
Apr 1, 2019
Merged

Fixed Pivot to show the correct PivotItem#8501
dzearing merged 1 commit into
microsoft:masterfrom
sebastianmattar:Fix-PivotItems

Conversation

@sebastianmattar

@sebastianmattar sebastianmattar commented Mar 27, 2019

Copy link
Copy Markdown
Contributor

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

There was a problem when using JSXs expressions to hide PivotItems the indexes were wrong. Modified the test case to cover this.

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

when JSX expressions are used
@size-auditor

size-auditor Bot commented Mar 27, 2019

Copy link
Copy Markdown
Bundle test Size (minified) Diff from master
Pivot 168.476 kB ExceedsBaseline     15 bytes

ExceedsTolerance  Exceeds Tolerance     ExceedsBaseline  Exceeds Baseline     BelowBaseline  Below Baseline     1 kB = 1000 bytes

@jdhuntington

Copy link
Copy Markdown
Contributor

Thanks @sebastianmattar .
I would like to better understand the use-case and expected behavior before merging. After playing with defaultSelectedIndex and null children, what is committed seems like the correct semantics to me. I feel like this PR makes the behavior less obvious.

I modified one of the examples and pasted at https://gist.github.com/jdhuntington/1f2bf3f1c0db22f16cc1ebf61b4c3b3f - with this PR in place and looking at the example, toggling the additional pivot on and off seems like a less favorable UX than what is already committed.

Would you be able to communicate what you expect to happen via a test other than a snapshot test?

@sebastianmattar

Copy link
Copy Markdown
Contributor Author

Hi!
My PR fixes the problem, that when some children are null, the children of the wrong PivotItem will be rendered.
The reason was that keyToIndexMapping was created by directly iterating over the children but _renderPivotItem relies on using React.Children.toArray() (which drops null values). I will have a look at your Gist later...

@sebastianmattar

Copy link
Copy Markdown
Contributor Author

Alright, I had a look at your Gist and converted it to CodePen. There, the bug can be reproduced: https://codepen.io/anon/pen/BEBgjO

If you click on "Shared with me" you will note that the Pivot #3 will not be rendered if isElementVisible===false. This is the bug my PR will fix.

@khmakoto

khmakoto commented Apr 1, 2019

Copy link
Copy Markdown
Member

Closing and reopening to trigger new build job in Azure DevOps.

@khmakoto khmakoto closed this Apr 1, 2019
@khmakoto khmakoto reopened this Apr 1, 2019
@dzearing
dzearing merged commit c5bd15c into microsoft:master Apr 1, 2019
@msft-github-bot

Copy link
Copy Markdown
Contributor

🎉office-ui-fabric-react@v6.164.1 has been released which incorporates this pull request.:tada:

Handy links:

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants