[Tabs] Remove height compensation

[denispaluca]

Pull Request

📖 Description

The height of the FluentTab is derived from the height of the FluentTabs component. This is done by subtracting 40px from the height of FluentTabs. The 40px is the height of the fluent tabs list. This is not necessary as the FluentTab is rendered inside the tabpanel part of the fluent-tabs web component which already accounts for this. (fluent-tabs is a grid with tablist taking the first row and tabpanel taking the rest.)

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

✅ Checklist

General

• I have added tests for my changes.
• I have tested my changes.
• I have updated the project documentation to reflect my changes.
• I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

• I have added a new component
• I have added Unit Tests for my new component
• I have modified an existing component
• I have validated the Unit Tests for an existing component

⏭ Next Steps

[dvoituron]

What the current problem with the existing code (40px)? Why did you created this PR?

[vnbaaij]

My question exactly 😁

But the correction is actually needed. This is the current situation:

The fluent-tabs grid is exactly as high as the fluent-tab-panel (where the 40px are substracted)

This is the situation if we would merge your PR:

The fluent-tabs grid would be 40px higher than the fluent-tabs-panel which could lead to layout issues for some cases

[denispaluca]

@vnbaaij You are correct in the example you gave above where the height is given in pixels. But when the height is given as a percentage then the 40px subtraction is not needed.

Maybe the height of FluentTab should be set to 100% be default. It would take 100% of tabpanel (the slot in the web component) which takes the rest of the height from the fluent-tabs. Then you would not need to set it this way.

[denispaluca]

@dvoituron My issue is that I am setting FluentTabs to the height 100%, the FluentTab now has the height 100% - 40px. In this case 100% does not refer to the height of the parent of FluentTabs but the the height of the tabpanel part. The height of tabpanel is already the height of FluentTabs - 40px since fluent-tabs is a grid where with grid-template-rows: auto 1fr where tabpanel is on the second row. This means that the height of the FluentTab will now be 100% - 80px. So i lose 40px of height.

[vnbaaij]

@dvoituron My issue is that I am setting FluentTabs to the height 100%, the FluentTab now has the height 100% - 40px. In this case 100% does not refer to the height of the parent of FluentTabs but the the height of the tabpanel part. The height of tabpanel is already the height of FluentTabs - 40px since fluent-tabs is a grid where with grid-template-rows: auto 1fr where tabpanel is on the second row. This means that the height of the FluentTab will now be 100% - 80px. So i lose 40px of height.

Ok, this makes it clear what you are trying to achieve. As I showed in my comment before, there is a situation where the -40px is needed. The PR needs to take both situations into account. Can you change it so it cater both options? Easiest way is, I think to add an extra .AddStyle line that checks if a height ending with a '%' is supplied.

[dvoituron]

OK. Clearer.
But the current PR can be a breaking change for other developers.

You could add a CSS variable --tab-header-height (or better name) containing the 40px value. So in your case, you could set this value to 0px.

[denispaluca]

@vnbaaij @dvoituron Ok I will change it. What speaks against just setting the height of the FluentTab always to 100% ? As I mentioned it the FluentTab will be inside of tabpanel which always takes the rest of the space. So it would not matter if the height of FluentTabs is in pixels or percent.

[vnbaaij]

@vnbaaij @dvoituron Ok I will change it. What speaks against just setting the height of the FluentTab always to 100% ? As I mentioned it the FluentTab will be inside of tabpanel which always takes the rest of the space. So it would not matter if the height of FluentTabs is in pixels or percent.

fluent-tab and fluent-tab-panel are separate web components and are rendered besides each other in the DOM panel is not a child of the tab.

[denispaluca]

@vnbaaij If you open the shadow-root of the fluent-tabs you will see that all fluent-tab-panel are slotted inside the tabpanel part of fluent-tabs.

[vnbaaij]

Ah, ok. I understand what you mean.

If you set the height on just the fluent-tabs component, the panels will not take over that height:

In this image I hovered over the first fluent-tab-panel in the tabpanel slot. As you can see it is then only 20px high

With the code we have, that fluent-tab-panels actual are resized to match the available height:

Also hovering the first fluent-tab-panel in the tablist slot

I think not making them the same size can lead to unintended/unexpected rendering results. Am I wrong there?

[denispaluca]

@vnbaaij My suggestion was to set the height of FluentTab (fluent-tab-panel) to 100%. Meaning it would take the the full height of the tabpanel part. The tabpanel should take the rest of the height of fluent-tab because fluent-tab is a grid with grid-template-rows: auto 1fr (the 1fr part being take over by tabpanel).

Here I'm hoving on fluent-tab (height: 300px) where fluent-tab-panel has a height of 100%.

[vnbaaij]

So do it like this?

 protected string? StyleValue => new StyleBuilder(Style)
        .AddStyle("height", "100%", () => !string.IsNullOrEmpty(Owner?.Height))
        .AddStyle("overflow-y", "auto", () => !string.IsNullOrEmpty(Owner?.Height))
        .Build();

[denispaluca]

@vnbaaij Implemented the change as above 👍