Skip to content
This repository has been archived by the owner on Jan 6, 2025. It is now read-only.

poll: what should be the default behavior for fxFlex? #689

Closed
CaerusKaru opened this issue Mar 22, 2018 · 34 comments · Fixed by #884
Closed

poll: what should be the default behavior for fxFlex? #689

CaerusKaru opened this issue Mar 22, 2018 · 34 comments · Fixed by #884
Assignees
Labels
discussion Further discussion with the team is needed before proceeding has pr A PR has been created to address this issue
Milestone

Comments

@CaerusKaru
Copy link
Member

This issue tracks the possibility of changing the default behavior for the fxFlex directive. Currently, the behavior is as follows:

  • When the input is an integer, this defaults to a percentage value input for fxFlex
  • When the input is any other singular value, it's interpreted as the intended flex-basis value and results in flex: 1 1 <input> with min-width and max-width applied (assuming row format)
  • When the input is three values, it's interpreted as a direct translation to the CSS flex property, with min/max-width applied according to the algorithm specified here
  • When the input is longer than three values, it is truncated to the first three values

I'll list some alternative options for fxFlex behavior below, please vote for your preferred option, or feel free to add your own. If you like the current behavior, please do not add comments, but simply vote for this comment.

@CaerusKaru CaerusKaru added the discussion Further discussion with the team is needed before proceeding label Mar 22, 2018
@CaerusKaru
Copy link
Member Author

CaerusKaru commented Mar 22, 2018

Option 1)

The fxFlex directive should mimic as close as possible the CSS flex property, with the following syntax:

  • When the input is an integer, this is interpreted as the CSS flex shorthand and is applied as such
  • When the input is any other singular value, it's interpreted as the intended flex-basis value and results in flex: 0 1 <input> (the default CSS flex values for grow and shrink)
  • When the input is three values, it's interpreted as a direct translation to the CSS flex property
  • When the input is longer than three values, it is truncated to the first three values

@CaerusKaru
Copy link
Member Author

Option 2)

The same as option 1, but min/max-width is applied as it is with the current default

@CaerusKaru
Copy link
Member Author

Option 3)

The same as option 1, but the min-width/height and max-width/height can be specified at the end of the fxFlex declaration as follows:

fxFlex="grow shrink basis [min] [max]", where none means don't apply max/min
fxFlex="0 1 500px 200px 500px"

@CaerusKaru
Copy link
Member Author

CaerusKaru commented Mar 22, 2018

Option 4)

The same as the current default, but the min-width/height and max-width/height can be specified at the end of the fxFlex declaration as follows:

fxFlex="grow shrink basis [min] [max]", where none means don't apply max/min
fxFlex="0 1 500px 200px 500px"

@adamdport
Copy link

Option 5) The same as option 3 but the default min/max is none

@CaerusKaru CaerusKaru added this to the v6.0.0-beta.15 milestone Mar 23, 2018
@CaerusKaru
Copy link
Member Author

Update: once beta.14 gets released, I'll close out the results of this poll and start working on the implementation. Then we'll take comments on the PR before it gets merged.

@CaerusKaru
Copy link
Member Author

Talked about this offline with @ThomasBurleson and here's our consensus:

  • Changing the default behavior of fxFlex constitutes a massive breaking change, one that would require tooling to get around (much like the material-prefix-adapter tool for when Material went from md to mat prefixes). We're not willing to invest in that at the moment, but we are willing to consider that as an adoption plan
  • In the meantime, we are willing to add this new configuration as a token so that you can enable it in your apps as an opt-in. This will likely see a release either in beta.16 or beta.17
  • A workaround for now is to simply declare the full flex instead of the shorthand, which is currently flex: 2 -> flex: 2 1 0%. Alternatively, you can use the fxGrow attribute
  • We don't have a workaround for max/min-width settings other than to use !important on a class for the element, but this will be part of the new configuration token added in beta 16/17

Hope that helps, and as always feel free to post any comments/concerns below.

@gedclack
Copy link

gedclack commented Apr 4, 2018

Hi, so the beta.14 is out, after I tried it, my flex-layout seems to be a bit broken..
I have something like this and it's works in beta.13 but broken in beta.14 :

<div fxLayout="column" fxFill>
  <div> ...toolbar... </div>
  <div fxFlex style="overflow-y : auto"> 
    .. fill the remaining space, respecting the toolbar's height above .. 
    .. and also a long list of items here ..
  </div>
</div>

what breaks? the toolbar shrink caused by the long item list below it.

so with beta.14 I have to add fxFlex="none" to the Toolbar div to fix it.
is this by design ?

UPDATE 25 April 2018 : stackblitz link
https://stackblitz.com/edit/angular-flex-layout-beta14-bug

@CaerusKaru
Copy link
Member Author

Can you encapsulate this in a StackBlitz demo on a separate issue? It might be a bug introduced by the flex basis changes made in beta.14

@gedclack
Copy link

gedclack commented Apr 4, 2018

Sure, will do it soon.. I thought this is by design.

@mtraynham
Copy link

I've also attempted an upgrade to beta.14 and it broke most of my app. The app itself is a full screen experience with custom scrolling, so often I'm using fxFlex to fill the remaining space of parent components, whether they be row or column layouts. This app is ported from Angular 1 material, where I was originally using layout-fill from the old library until I realized it was completely broken on Safari.

After this upgrade, I think most of my issues stem from display: flex; flex-direction: row no longer being implicitly applied to child components. I'm not entirely sure if it's related to 3cfafd1, but that might be my best guess.

I've also had somewhat of a hack in place that set's minWidth: 0 when fxFlex is applied within a row layout, using the following scss. I've never really liked it, but the Material 1 library used to apply it, and it worked.

*[fxlayout],
*[fxlayout='row'],
.layout-row,
*[style*='display: flex'][style*='flex-direction: row'] {
    > *[fxflex],
    > .flex,
    > ui-view,
    > *[style*='flex: 1 1 1e-09px'] {
        min-width: 0;
    }
}

I feel like this is about to be the 3rd time I've been burned by these layout changes. Can you please advise on what fxFlex/fxLayout should do in terms of a full screen app and filling vertical space to accommodate custom scrolling?

And how should components interact outside their own template and within their own template?

@CaerusKaru
Copy link
Member Author

@mtraynham Flex styles are no longer implicitly applied from fxFlex to its parent. This was part of #670 and can be re-enabled by providing the ADD_FLEX_STYLES InjectionToken as follows:

import {ADD_FLEX_STYLES} from '@angular/flex-layout';

{provide: ADD_FLEX_STYLES, useValue: true}

My best guess at a fullscreen app filling vertical space would be something like this:

styles.css

html, body {
	height: 100%;
	width: 100%;
}

app.component.html

<div fxLayout="column" fxFill>
	<div fxFlex>
		<router-outlet></router-outlet>
	</div>
</div>

@CaerusKaru
Copy link
Member Author

Also, we no longer use the 1e-9px hack, we instead set the flex-basis to 0% in row mode and auto for column mode.

@mtraynham
Copy link

So ADD_FLEX_STYLES got me fairly back to normal. I think the rest of my cases are using a fxLayout="column" with an fxFlex child that no longer consumes the entire space. What's the justification for auto in column mode and not 0%?

@CaerusKaru
Copy link
Member Author

It's a workaround for column layouts in IE and Safari as documented by FlexBugs.

@mtraynham
Copy link

mtraynham commented Apr 5, 2018

Alright @CaerusKaru, I have my app back in it's original state... For some reason fxLayout="column" and a child of fxFlex, which as you've suggested gives it flex-basis of auto didn't fill the parent for me. I also tried fxFlex="0%" on those children and that worked for Chrome, but not Firefox. I instead left my code as is with simple fxFlex and used the following scss to correct it (which goes back to the original hack). This seems to work for Chrome/Firefox/Safari, but I haven't tried IE.

*[fxlayout='column'] > *[fxFlex] {
    flex-basis: 0.000000001px !important;
}

@CaerusKaru
Copy link
Member Author

@mtraynham Can you put what you're seeing in a StackBlitz, where the child fxFlex doesn't expand to fill the parent? I'm not seeing that in any of our X-browser testing. I've tested with Safari 10/11, IE11, Firefox 56, and Chrome latest

@buu700
Copy link

buu700 commented Apr 10, 2018

Is there a recommended way to get the Beta 13 behavior back in Beta 14? I'm not doing anything flex-related directly in CSS, but Beta 14 still breaks my existing layouts in a lot of weird ways that don't seem easy to work around.

Edit: Didn't notice all the new messages since I last opened this thread. It sounds like ADD_FLEX_STYLES will do what I want?

@CaerusKaru
Copy link
Member Author

It depends on what's different in your layouts. As mentioned above, the only stark change you should see is that fxFlex no longer applies flex-direction to its parent by default. You can re-enable this by adding:

import {ADD_FLEX_STYLES} from '@angular/flex-layout';
{provide: ADD_FLEX_STYLES, useValue: true} to the list of providers in the module where you import FlexLayoutModule

@buu700
Copy link

buu700 commented Apr 10, 2018

Thanks! I'll try that out.

@mrjpeterj
Copy link

I am also seeing the same as @mtraynham in that I have fxLayout="column" and using fxFlex now doesn't adjust the content to fit the remaining space (On FireFox and Edge).

In these cases there are fixed header and footer elements and the content using fxFlex is longer than the remaining space. So the aim is for that content to be placed into the gap and have a scrollbar.
With the change to flex: 1 1 auto it just displays at its natural size and pushes the footer off the screen (On FireFox and Edge). Reverting back to flex: 1 1 1e-9px as per beta.13 displays as required.

I'll try and create a StackBlitz showing it

@mrjpeterj
Copy link

Here it is https://stackblitz.com/edit/angular-xmaew3

The issue only happens with a bit of nesting, which is generally what you get anyway when using router-outlet with a fixed toolbar and the router content expanding to fit the rest of the screen

@CaerusKaru
Copy link
Member Author

@mrjpeterj This is probably something we can consider. My memory was that we made the change because it affected row/columns in Safari or IE11, but it looks like 0px works just as well for both (in addition to fixing your problems). I will investigate further and see if a patch is warranted.

@gedclack
Copy link

gedclack commented Apr 25, 2018

@CaerusKaru Here is the stackblitz :
https://stackblitz.com/edit/angular-flex-layout-beta14-bug
I can fix the layout by adding fxFlex="none" to the affected container, but it will be a lot of work in my project.

Please tell me if I did it the wrong way...

@buu700
Copy link

buu700 commented May 2, 2018

@CaerusKaru, I just tested out the ADD_FLEX_STYLES workaround with 6.0.0-beta.15, and it didn't fully fix the problem. (Still have to do more extensive testing/comparison to determine the extent to which it did mitigate the problem, if at all.)

Luckily stacking that with @mtraynham's flex-basis trick seems to do the job for now.

@CaerusKaru
Copy link
Member Author

@buu700 This will be fixed by #729

@CaerusKaru
Copy link
Member Author

I've been thinking a lot more about this since I read #825. How about adding to the LAYOUT_CONFIG token, allowing users to provide their own style-building functions and default values? E.g.

{
	defaults: {
		fxFlex: '1',
	},
	styleBuilders: {
		flex: (val: string) => {},
		flexGap: (val: string) => {},
	}
}

@CaerusKaru CaerusKaru added the has pr A PR has been created to address this issue label Nov 11, 2018
@CaerusKaru CaerusKaru self-assigned this Nov 11, 2018
@CaerusKaru
Copy link
Member Author

I've encapsulated the style builders from the above comment into #884. This allows you to provide a custom override implementation, e.g. for fxFlex if you want one. I think this is the sleekest option instead of overriding the defaults for everyone, especially long-time users.

CaerusKaru added a commit that referenced this issue Nov 14, 2018
This change decouples the style-generation from the actual directives, meaning that the library is now composed of the following:

* Directives that respond to inputs and `matchMedia` events
* Style generation providers that return styles when triggered by directives

This allows for end-users or library authors to provide their own unique style generation (even by borrowing or extending our existing library code) for any directive. This is entirely non-mandatory for use of the `BaseDirective`, since the `BaseDirective` need not always use a de-coupled style provider to function.

The canonical example is the following:
```ts
@Injectable()
export class CustomStyleBuilder extends StyleBuilder {
  buildStyles(input: string) {
	return {
		'style1': 'value1',
	};
  }
}

@NgModule({
	...
	providers: [
		provide: <the style builder to orverride, e.g. FlexStyleBuilder>,
		useClass: CustomStyleBuilder,
	],
})
export class MyAppModule {}
```

Fixes #689
@kghost
Copy link

kghost commented Dec 25, 2018

Please document it. After digging tons of issues/prs (#737, #438, #729, #689, #884), I still have no idea of how to omit min/max-width set on the element.

@CaerusKaru
Copy link
Member Author

StyleBuilders are documented on the Wiki here: https://github.com/angular/flex-layout/wiki/Style-Builders. Otherwise, docs PRs are welcome.

@ronniebarker
Copy link

It's still not clear how to disable max-width and min-width together. Looking at the code, they seem to be coming up when there is a wrap element as the parent. It still doesn't make sense that the min and max should be set together to the third value of fxFlex... I don't think that extending with a styleBuilder is appropriate. My work-around is to revert to css as there seems to be little benefit to fxFlex='1 1 100px' over style='flex: 1 1 100px'

@simeyla
Copy link

simeyla commented Aug 30, 2019

@ronniebarker I'm still confused too :-(

Flex-Layout works for me very well nearly every time, but fxFlex="1 1 10em" confuses me every time!

I kind of want to be able to do this with my current situation:

// makes the grow important - so doesn't apply max-width (would still apply min-width)
fxFlex="1! 1 8em"

// makes the shrink important - so doesn't apply min-width (would still apply max-width)
fxFlex="1 1! 8em"

// makes the grow and shrink important - so doesn't apply min / max width
fxFlex="1! 1! 8em"

Would this micro-syntax be easy for me to implement with a style builder?

I think most people finding this question have just ONE thing they need to prevent a style for - and the thought of learning about style builders is just too much work. Of course I could revert to [style.flex], but for consistency that doesn't work well - especially in my current component where flex is dynamic ([fxFlex]="calculateFlex()")

@CaerusKaru
Copy link
Member Author

For the record, I'm not opposed to providing some "prebuilt" StyleBuilders. If there is consensus on this thread on an algorithm – and a name – I'd be happy to add it in time for the next release.

@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 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion Further discussion with the team is needed before proceeding has pr A PR has been created to address this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants