Pivots move to overflow set when limited space - Initial PR for design review #11270
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
msnyder-msft
changed the title
|
@msnyder-msft can you add a changefile? Just run |
|
i know this just an early code review, but it'd be great to get it passing so that we can see the rendered output as well. Just need to add a changefile to get ci kicked off |
Sorry for the delay. It's pushed now. Didn't realize that was a problem for design review. |
|
It looks like the build has failed with the following error: ● Pivot › renders link Pivot correctlyGood news, it appears that I might be able to help with automatically recovering from this error}. ✔️ Recovery has been initiated by @micahgodbolt at 2019-11-22T22:47:24Z (oufr - update snapshots)*. 1 Please note, automated recovery may not always be the right action to take. Please evaluate the error carefully first.
|
|
Dear humans, I have kicked off a recovery build at @micahgodbolt's instruction. You can monitor progress here. |
|
@msnyder-msft its not really REQUIRED for a review, but we currently fail builds that don't have one, and its great to see the finished build in our PR deploy site. |
@micahgodbolt That's really neat! I didn't realize it would do that. Will it still deploy if snapshots are failing? Because I haven't updated the tests yet. |
|
Looks like it didn't resolve all of the snap issues. cd into packages/office-ui-fabric-react and run |
@micahgodbolt Thanks! I pushed the snapshots now. |
|
@msnyder-msft can you push those updated snaps so we can get a PR preview? |
|
oh, snaps seem done. there are some linting issues popping up about your imports
|
|
@micahgodbolt I've pushed the fixes for those 2 files. We'll see if it's happier now. The output from that lint command (lerna run build --stream -- --lint) is so noisy that it's really hard to see where the actual issues are at so thanks for highlighting the important stuff. |
|
I saw issues about physical file imports. Don't include the file extension in your imports. |
Asset size changes
Baseline commit: 347134e1debf871386dc689e3f675522835bfec5 (build) |
@micahgodbolt I just pushed an update to fix the errors about the API. I think everything should be happy now except for jest which is complaining about force stopping. |
Component Perf AnalysisNo significant results to display. All results
|
|
Looks like we've got a passing build :D We are getting visual test errors, and I can tell you why.
Now that we're using resizeGroup to fit the pivot into the given space......this means that if the parent is not a block level element, it'll keep shrinking to fit the contents.....and the contents will keep shrinking to fit the parent. So you get a fully collapsed pivot. This will be a concern going forward, as this change could be considered breaking as pivots no longer automatically take up all the space expected. |
|
@micahgodbolt I see you started the review thread over email. I had seen those failing tests but before investing too much in fixing them I wanted to be sure the major design pattern was acceptable to folks. Once folks are happy with that, I'll get those discrepancies resolved. |
|
@msnyder-msft trying to remember if you were on this thread about our thoughts here, but to sum up: Making this change is probably going to be too disruptive for anyone expecting pivots to take up 100% of the space that it can. What we'd like to recommend is to update the pivots to allow for an onRender function to replace the existing pivots with custom pivots that you could customize in the way that you did. We can even include an example of how you can do it (depending on complexity). So basically, take the work you did into an onRenderPivotItems (or something similar), and update the pivot to support that custom render. Happy to give you a hand with how this would work. I know it has come up a few times. |
|
@micahgodbolt thanks for the summary here. I was waiting for an outcome on that thread before moving forward. I understand the general idea but not sure I am completely clear where this logic would live now. |
|
@msnyder-msft it'd look something like this <Pivot
onRenderPivotLinks={(linkCollection, selectedKey) => {
return <ResizeablePivotLinks linkCollections={linkCollection} selectedKey={selectedKey} />;
}}
linkFormat={PivotLinkFormat.tabs}
>
<PivotItem headerText="Foo">
<Label>Pivot #1</Label>
</PivotItem>
<PivotItem headerText="Bar">
<Label>Pivot #2</Label>
</PivotItem>
<PivotItem headerText="Bas">
<Label>Pivot #3</Label>
</PivotItem>
<PivotItem headerText="Biz">
<Label>Pivot #4</Label>
</PivotItem>
</Pivot> |
|
Also, you don't necessarily need to write the control in Fabric. We just need the Pivot set up to be flexible enough to support that custom render function |
|
@micahgodbolt ok that makes sense. So you're saying to make limited changes in Fabric and that |
|
Bingo :) How can we modify the current pivot without breaking existing API to support rendering a customized set of links. Then you can hack on that in your own repo, and if you come up with a really solid, stable solution we can look at adding that to the OUFR repo |
|
@msnyder-msft is take a new approach that will make the change less disruptive. I'm going to close this PR down until he's ready for a new review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ yarn changeDescription of changes
Today, pivots are not very responsive. When many tabs are shown and take up a lot of horizontal space, they don't adapt when the page/container size changes.
This change set is intended to add a resize group around the pivot and pushes items into an overflow set when there isn't enough room to show all the tabs.
This is just an initial change set to get some input on designs and things to consider. I'd love to hear your thoughts.
I also haven't touched the tests, I know there's going to be a bunch of work to do there.
Focus areas to test
Pivots
Microsoft Reviewers: Open in CodeFlow