Skip to content
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.

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

[NG] Enable tab content to scale to tabs container (#3279) #3298

Merged
merged 2 commits into from Apr 17, 2019

Conversation

Projects
None yet
3 participants
@Jinnie
Copy link
Contributor

commented Apr 15, 2019

The introduction of block-level section element had introduced the
need to explicitly manage the container height. Reverting it to
inline will allow direct visibility between the tab content and tabs
parent container.

Signed-off-by: Ivan Donchev idonchev@vmware.com

[NG] Enable tab content to scale to tabs container (#3279)
The introduction of block-level section element had introduced the
need to explicitly manage the container height. Reverting it to
inline will allow direct visibility between the tab content and tabs
parent container.

Signed-off-by: Ivan Donchev <idonchev@vmware.com>
@Shijir

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Hi Ivan, as the tabs component is a layout component, shouldn't we rather have block, flex, grid as a display property instead of inline? Secondly, it seems like our tabs component works just fine if user sets the height on the tab content:
https://stackblitz.com/edit/clarity-tabs-with-heigth-ph3zyc?file=src%2Fapp%2Fapp.component.css

@Jinnie

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Hi @Shijir,
1.0.x is fine, the problem is with 1.1.x - https://stackblitz.com/edit/clarity-tabs-with-heigth-1-1-x?file=package.json
About if it would be better to have the tabs as block level - it probably would, but it will be a breaking change, as it will require users to update their current code. This specific change is more aimed at restoring the previous way this component worked.
If you need more information I can point you to the slack discussion we had on this topic, where both these options were compared and discussed.

@Shijir

Shijir approved these changes Apr 16, 2019

@Shijir

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

As per our discussion, I've given it :shipit: . Please make sure it works across different browsers. Thank you :)

IE 11 compatibility update
Signed-off-by: Ivan Donchev <idonchev@vmware.com>

@Jinnie Jinnie merged commit e818018 into vmware:master Apr 17, 2019

3 of 6 checks passed

Header rules No header rules processed
Details
Pages changed 6 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@Jinnie Jinnie deleted the Jinnie:topic/tabs-fix branch Apr 17, 2019

@Jinnie

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Will close the related issue after merging in the v1 branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.