Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

clshortfuse
Copy link
Contributor

@clshortfuse clshortfuse commented Aug 29, 2016

Certain browsers had issues with resizing tab content md-tabs' parent
would change size or have an unspecified size during creation (for
example in a dialog). Converting to flexbox solves this issue.

  • Set md-tab-content to flexbox
  • Set md-tab-content's div to fill parent
  • Set md-pagination-wrapper to flexbox

Fixes #9206, #9704, #9779

@clshortfuse clshortfuse added this to the 1.1.1 milestone Aug 29, 2016
@clshortfuse clshortfuse added the needs: review This PR is waiting on review from the team label Aug 29, 2016
@topherfangio
Copy link
Contributor

I feel like this might break some existing users who expect it to be block?

Also, have you tested this across all the browsers we support to make sure it works as expected?

@clshortfuse
Copy link
Contributor Author

clshortfuse commented Aug 29, 2016

@topherfangio OK on Chrome, Firefox, IE 10, IE11 and Edge.

The reason I didn't just use display:flex was so we can use our layout engine which has more predictable results. I have yet to test on on Safari which honestly might be the only browser to present issues due to its flex-shrink bug. https://bugs.webkit.org/show_bug.cgi?id=146020

@clshortfuse clshortfuse added the needs: manual testing This issue or PR needs to have some manual testing and verification done label Aug 29, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.2, 1.1.1 Aug 30, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.3, 1.1.2 Sep 7, 2016
@ThomasBurleson ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Sep 7, 2016
@topherfangio topherfangio removed the needs: review This PR is waiting on review from the team label Sep 12, 2016
@clshortfuse clshortfuse removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: manual testing This issue or PR needs to have some manual testing and verification done labels Oct 14, 2016
@clshortfuse
Copy link
Contributor Author

I've reworked this to not use mdLayout and instead the display:flex in CSS.

This should not break layouts since user content is wrapped by md-tab-content, which is what we are modifying here. Despite we are adding CSS to > .div, the flex rule only applies to its relation to its parent, not its children.

We are also using flex: 1 0 to avoid any possible issues due to Safari's flexbug rendering issue: https://github.com/philipwalton/flexbugs#1-minimum-content-sizing-of-flex-items-not-honored

@ThomasBurleson ThomasBurleson changed the title fix(tabs) support flexible content layout fix(tabs): support flexible content layout Oct 14, 2016
@clshortfuse clshortfuse force-pushed the tabs-flexContent branch 3 times, most recently from c78b71f to 45864ab Compare October 14, 2016 02:53
@clshortfuse clshortfuse added the needs: review This PR is waiting on review from the team label Oct 18, 2016
Certain browsers had issues with resizing tab content md-tabs' parent
would change size or have an unspecified size during creation (for
example in a dialog). Converting to flexbox solves this issue.

* Set md-tab-content to flexbox
* Set md-tab-content's div to fill parent
* Set md-pagination-wrapper to flexbox

Fixes angular#9206, angular#9704, angular#9779
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.2, 1.1.3 Oct 24, 2016
Copy link
Contributor

@topherfangio topherfangio left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ThomasBurleson ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Oct 24, 2016
@kara kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Nov 15, 2016
@kara kara merged commit d89a682 into angular:master Nov 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge ready This PR is ready for a caretaker to review ui: CSS ui: flexbox
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants