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

fxFlex: is not configuring min-height properly for 'column' layouts. #162

Closed
ThomasBurleson opened this issue Feb 7, 2017 · 13 comments
Closed
Assignees
Labels
bug P3 Important issue that needs to be resolved related: angular-core

Comments

@ThomasBurleson
Copy link
Contributor

For <div fxLayout="column" fxFlex> the min-height is not configured properly and the UX is rendered as:

screen shot 2017-02-06 at 10 17 17 pm

With inline styles of

<div fxLayout="column" 
          fxFlex
          style="flex: 1 1 100%;min-height:100vh; background-color:yellow">
</div>

then the layout is correct as follows:

screen shot 2017-02-06 at 10 14 46 pm

@ThomasBurleson ThomasBurleson added this to the v2.0.0-beta.5 milestone Feb 7, 2017
@ThomasBurleson ThomasBurleson self-assigned this Feb 7, 2017
@ThomasBurleson
Copy link
Contributor Author

ThomasBurleson commented Feb 7, 2017

Refs PR #160

@somombo
Copy link
Contributor

somombo commented Feb 9, 2017

is this now fixed via #160 ?

@ThomasBurleson
Copy link
Contributor Author

ThomasBurleson commented Feb 9, 2017

No, this is an extra logic condition we need to add/account-for.

@somombo
Copy link
Contributor

somombo commented Feb 9, 2017

@ThomasBurleson could you post the plunk ur in screenshot.. i wanted to see if could play with it a little

@joshwiens
Copy link
Contributor

joshwiens commented Feb 9, 2017

http://plnkr.co/edit/WzfbnAaRBxH8H67B5Di4?p=preview

@ThomasBurleson ThomasBurleson modified the milestones: v2.0.0-beta.5, v2.0.0-rc.1 Feb 10, 2017
@somombo
Copy link
Contributor

somombo commented Feb 19, 2017

Setting min-height: 100vh; seems to be resolving this, but I am concerned about when exactly this will be applied e.g. suppose we say "if fxLayout="column", then add min-height: 100vh;" ... This may conflict with other times when we dont want there to be a min-height to be set e.g. #160

So under what programmatic circumstances exactly do we want this or a similar resolution to apply?

By the way, the flex: 1 1 100%; bit in the proposed fix, is not necessary. Output remains exactly the same without this.

@ThomasBurleson
Copy link
Contributor Author

ThomasBurleson commented Feb 19, 2017 via email

@ThomasBurleson ThomasBurleson modified the milestones: v2.0.0-rc.1, v2.0.0-rc.2 Mar 2, 2017
@ThomasBurleson ThomasBurleson modified the milestones: v2.0.0-rc.2, v2.0.0-rc.3 Mar 9, 2017
@ThomasBurleson
Copy link
Contributor Author

Related to #135, #201

@somombo
Copy link
Contributor

somombo commented Mar 30, 2017

Refer to the Angular feature propasal for declaratively adding directives to a host component

I have been thinking for a while about this problem and I think if the above feature was implemented, then it would resolve this and other problems in a trivial way.

@ThomasBurleson, What follows below is a detailed elaboration of how the feature could work and what I think are benefits of the feature. My hope is that perhaps you may help champion an effort to get it adopted by the Angular team. It appears that there is great excitement about the prospects of the feature; it's the third most thumbed-up among all open angular issues! However there has not been any feedback from the angular team (positive/negative) about any considerations/limitations related to potentially implementing it.

For what follows, I'll assume that the feature above is implemented by adding a "hostDirectives" metadata property to components/directives (See other proposals for implementation). Such a property would allow one to declare what directives should be added to a component so that for example:

Suppose there are two directives defined as follows:

@Directive({selector: "dir1"}) class Dir1 {}
@Directive({selector: "dir2"})  class Dir2 {}

Then the following:

@Component({
  // ...
  selector: "my-component",
  hostDirectives: [Dir1, Dir2] // proposed by https://github.com/angular/angular/issues/8785
  // ...
}) class MyComponent {}
<my-component id="firstComp"></my-component>

<my-component id="secondComp"></my-component>

Would result in exactly the same thing as:

@Component({
  // ...
  selector: "my-component",
  // the current angular way has no "hostDirectives" for combining directives
  // ...
}) class MyBareComponent {}
<!-- ... This is *not* DRY ...  -->
<my-component dir1 dir2 id="firstComp"></my-component>

<my-component dir1 dir2 id="secondComp"></my-component>

Hence if the feature got adopted, then the issue herein, would be a simple matter of:

@Component({
  selector: 'test-app',
  hostDirectives: [LayoutDirective],
  host: { 'fxLayout': 'column' },
  styles: [":host {min-height:100vh; background-color:yellow}"]
  template: `

  <!-- NOTE: With `hostDirectives`, It would no longer be necessary   to have 
  the outer div wrapper declaring the fxLayout='column' and min-height="100vh" -->
    
    <header class="flex-container"  fxLayout="row" fxLayoutAlign="center center">
      <!-- ... stuff ...  -->
    </header>
    
    <div class="flex-container" fxFlex fxFlexFill>
      <!-- ... stuff ... -->
    </div>
    
    <footer class="flex-container" fxLayout="row" fxLayoutAlign="center center">
      <!-- ... stuff ...  -->
    </footer>

  `,
})
export class TestApp { /* ... */}

This results in the root component (TestApp) having fxLayout directive applied to it.
This is particularly nice because it appears there is currently no non-convoluted way of achieving this (See my unanswered stackoverflow quesiton)[http://stackoverflow.com/questions/41618197/consolidating-two-directives-into-a-single-one].

The above would be a basic resolution... However, if we wanted to refine this, we could create a custom made directive specifically to solve the issue herein. Let's define this directive as follows:

@Directive({
  selector: 'fxLayoutFullPage',
  hostDirectives: [LayoutDirective],
  host: { 
    'fxLayout': 'column', 
    'style': 'min-height:100vh; background-color:yellow'
  }, 
}) class LayoutFullPageDirective {}

Then the problem could be solved as follows:

@Component({
  selector: 'test-app',
  hostDirectives: [LayoutFullPageDirective],
  template: "<!-- ... Same template as above ... -->"

})
export class TestApp { /* ... */}

Which is an easy thing to document for regular users to understand, if ever they ran into this #162 issue herein.

So far I have found alot of other very use applications of the proposed feature. Another example, is many times i have had to write something like

<my-components-wrapper fxLayout>
  <my-componet1 fxLayout="column" fxFlex="1 0 auto"><!-- ... --></my-componet1>
  <my-componet2 fxLayout="column" fxFlex="1 0 auto"><!-- ... --></my-componet3>
  <my-componet3 fxLayout="column" fxFlex="1 0 auto"><!-- ... --></my-componet4>

</my-components-wrapper>

See my realistic plunker example

It would be nice if I could easily define all of the my components in such a way that I could simply write the above as:

<my-components-wrapper>
  <my-componet1><!-- ... --></my-componet1>
  <my-componet2><!-- ... --></my-componet3>
  <my-componet3><!-- ... --></my-componet4>
</my-components-wrapper>

What do you think??

@ThomasBurleson ThomasBurleson added the P3 Important issue that needs to be resolved label Jun 22, 2017
@CaerusKaru
Copy link
Member

Updating with StackBlitz demo

@CaerusKaru
Copy link
Member

I've updated the demo and resolved this issue. The solution was to add html { height: 100%; } and fxFlexFill to the top-level div in test-shell. Finally a buffer was added between the header and the footer to add the yellow section @ThomasBurleson mentioned.

While this issue mostly serves as a "add flex directive to the host dynamically" issue, it's not needed for the original use case, and is being tracked on angular/angular instead. I'm closing this issue without objection. We can always open a new one if there's an Angular Layout-specific use case.

@simeyla
Copy link

simeyla commented Feb 18, 2019

General word of caution about vh (nothing to do with flex-layout directly)

Be super careful with 100vh, or vh units in general on mobile devices. Test on a real device early to avoid disappointment!

100vh can be taller than the visible screen, due to toolbars appearing / disappearing or shrinking. It was concluded by browser vendors not to ever change the vh due to such activity.

Note vh will change if you rotate the phone, but when scrolling up and down and toolbars appear/disappear then vh won't change.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug P3 Important issue that needs to be resolved related: angular-core
Projects
None yet
Development

No branches or pull requests

5 participants