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

fxLayoutGap and fxFlexOffset don't work together #692

Closed
mohlendo opened this issue Mar 29, 2018 · 4 comments · Fixed by #900
Closed

fxLayoutGap and fxFlexOffset don't work together #692

mohlendo opened this issue Mar 29, 2018 · 4 comments · Fixed by #900
Assignees
Labels
bug discussion Further discussion with the team is needed before proceeding has pr A PR has been created to address this issue P1 Urgent issue that should be resolved before the next re-lease
Milestone

Comments

@mohlendo
Copy link

It seems that only you can have either Gap or offset but not both. See reproducer for details:

https://stackblitz.com/edit/angular-material-flex-layout-seed-tqyekw

@CaerusKaru CaerusKaru self-assigned this Mar 29, 2018
@CaerusKaru CaerusKaru added bug P1 Urgent issue that should be resolved before the next re-lease discussion Further discussion with the team is needed before proceeding labels Mar 29, 2018
@CaerusKaru CaerusKaru added this to the v6.0.0-beta.16 milestone Mar 29, 2018
@CaerusKaru
Copy link
Member

This is an interesting issue, and I've been punting it for a reason. There are two obvious solutions to the problem:

  1. We can have fxLayoutGap check for fxFlexOffset children, and simply apply addition to get the margin values, then have fxFlexOffset be a no-op if it has a fxLayoutGap parent
  2. We can do the same, but reverse the direction

The only relevant question then becomes: how do we handle different responsive aliases, ie how do we coordinate the activated values? There may be an obvious solution to this too, but until I find it this issue remains in limbo.

@CaerusKaru
Copy link
Member

Ok this is actually a lot simpler than I led on about, because fxFlexOffset and fxLayoutGap work in opposite directions (i.e. when gap goes left, offset goes right). The issue is that gap is clearing out all of the margin stylings right after flex-offset gets applied, resulting in flex-offset not applying anything.

The fix, which will come after #900 lands, is to get pre-existing margin stylings and intelligently cancel them out. This part will require some thought, but it's a lot more straightforward than my previous comment.

@CaerusKaru
Copy link
Member

Actually this will be fixed by #900 as it turns out (turns out the refactor can't work without it 😄)

@CaerusKaru CaerusKaru added the has pr A PR has been created to address this issue label Dec 7, 2018
CaerusKaru added a commit that referenced this issue Dec 13, 2018
signed to work in symbiosis with the `MediaMarshaller`

The custom breakpoints story has changed significantly. Instead of extending directives that contain the default breakpoints and writing lengthy constructors, the process has been paired down to the following (i.e. for `fxFlexOffset`):

```ts
const inputs = ['fxFlexOffset.xss'];
const selector = `[fxFlexOffset.xss]`;

@directive({selector, inputs})
export class XssFlexOffsetDirective extends FlexOffsetDirective {
        protected inputs = inputs;
        }
        ```

Never again will a change in the base directive constructor require an entire rewrite of custom breakpoint directives. And registering a new directive no longer brings collisions and double-registrations competing with the default directives. Everything is separated and improved.

* All of the Grid and extended Flex directives have been updated to the new standards; namely, they have been refactored to the new API with `StyleBuilder`s. The notable exception is `show-hide`, which uses a `StyleBuilder`, but does not have a cache for the results.
> FxShowHide does not use a stylebuilder cache as to avoid complexity for cache-shifting based on the host `display` property. If needed, an internal cache can be easily added later.

* A number of APIs had the default values calculated in the directives instead of the `StyleBuilder`, meaning that it would be much harder to override by end-users. This has been fixed
* An issue where `fxLayoutGap` was canceling out `fxFlexOffset` has been corrected

* `MediaMonitor` (use `MediaMarshaller`)
* `ResponsiveActivation`
* `BreakpointX`
* `KeyOptions`
* `negativeOf`
* `BaseDirective` (use `BaseDirective2`)
* `BaseAdapter`

The minified size of the Angular Layout library has decreased *~38%* from **132KB** to **82KB**, a total savings of 50KB.  After the deprecated APIs are deleted (Beta.21), the build size will be reduced yet again.

It should also bring about performance improvements as a result of fewer RxJS subscriptions, memoized style lookups, and other API processing.

It is our hope that along with the added `StyleBuilder` functionality, and migration away from `ObservableMedia`, this represents the end-stage towards stability for Angular Layout.

Closes #903
Fixes #692
CaerusKaru added a commit that referenced this issue Dec 13, 2018
This design change marks a number of departures from the current Angular Layout configuration:

1. A number of APIs are deprecated in favor of a more polished, integrated API
2. A new semantics for creating custom breakpoint directives is introduced
3. A number of bugs caught along the way and design changes missed in past PRs are rectified

>  **PLEASE NOTE**: There will be **no end-user API changes** as a result of this PR.

Unless custom breakpoints are configured, devs should see no change in how the library usage. These changes will deliver notice significant size & performance improvements.

* `MediaMarshaller`
        A centralized data store for elements, breakpoints, and key-value pairs. The `MediaMarshaller` responds to mediaQuery changes and triggers appropriate Layout directives. This class also introduces a way to track changes for arbitrary elements and directive types
* `BaseDirective2`
        A new directive with stripped-down functionality from the old `BaseDirective` that is designed to work in symbiosis with the `MediaMarshaller`

The custom breakpoints story has changed significantly. Instead of extending directives that contain the default breakpoints and writing lengthy constructors, the process has been paired down to the following (i.e. for `fxFlexOffset`):

```ts
const inputs = ['fxFlexOffset.xss'];
const selector = `[fxFlexOffset.xss]`;

@directive({selector, inputs})
export class XssFlexOffsetDirective extends FlexOffsetDirective {
        protected inputs = inputs;
        }
        ```

Never again will a change in the base directive constructor require an entire rewrite of custom breakpoint directives. And registering a new directive no longer brings collisions and double-registrations competing with the default directives. Everything is separated and improved.

* All of the Grid and extended Flex directives have been updated to the new standards; namely, they have been refactored to the new API with `StyleBuilder`s. The notable exception is `show-hide`, which uses a `StyleBuilder`, but does not have a cache for the results.
> FxShowHide does not use a stylebuilder cache as to avoid complexity for cache-shifting based on the host `display` property. If needed, an internal cache can be easily added later.

* A number of APIs had the default values calculated in the directives instead of the `StyleBuilder`, meaning that it would be much harder to override by end-users. This has been fixed
* An issue where `fxLayoutGap` was canceling out `fxFlexOffset` has been corrected

* `MediaMonitor` (use `MediaMarshaller`)
* `ResponsiveActivation`
* `BreakpointX`
* `KeyOptions`
* `negativeOf`
* `BaseDirective` (use `BaseDirective2`)
* `BaseAdapter`

The minified size of the Angular Layout library has decreased *~38%* from **132KB** to **82KB**, a total savings of 50KB.  After the deprecated APIs are deleted (Beta.21), the build size will be reduced yet again.

It should also bring about performance improvements as a result of fewer RxJS subscriptions, memoized style lookups, and other API processing.

It is our hope that along with the added `StyleBuilder` functionality, and migration away from `ObservableMedia`, this represents the end-stage towards stability for Angular Layout.

Closes #903
Fixes #692
CaerusKaru added a commit that referenced this issue Dec 13, 2018
This design change marks a number of departures from the current Angular Layout configuration:

1. A number of APIs are deprecated in favor of a more polished, integrated API
2. A new semantics for creating custom breakpoint directives is introduced
3. A number of bugs caught along the way and design changes missed in past PRs are rectified

>  **PLEASE NOTE**: There will be **no end-user API changes** as a result of this PR.

Unless custom breakpoints are configured, devs should see no change in how the library usage. These changes will deliver notice significant size & performance improvements.

* `MediaMarshaller`
        A centralized data store for elements, breakpoints, and key-value pairs. The `MediaMarshaller` responds to mediaQuery changes and triggers appropriate Layout directives. This class also introduces a way to track changes for arbitrary elements and directive types
* `BaseDirective2`
        A new directive with stripped-down functionality from the old `BaseDirective` that is designed to work in symbiosis with the `MediaMarshaller`

The custom breakpoints story has changed significantly. Instead of extending directives that contain the default breakpoints and writing lengthy constructors, the process has been paired down to the following (i.e. for `fxFlexOffset`):

```ts
const inputs = ['fxFlexOffset.xss'];
const selector = `[fxFlexOffset.xss]`;

@directive({selector, inputs})
export class XssFlexOffsetDirective extends FlexOffsetDirective {
        protected inputs = inputs;
        }
        ```

Never again will a change in the base directive constructor require an entire rewrite of custom breakpoint directives. And registering a new directive no longer brings collisions and double-registrations competing with the default directives. Everything is separated and improved.

* All of the Grid and extended Flex directives have been updated to the new standards; namely, they have been refactored to the new API with `StyleBuilder`s. The notable exception is `show-hide`, which uses a `StyleBuilder`, but does not have a cache for the results.
> FxShowHide does not use a stylebuilder cache as to avoid complexity for cache-shifting based on the host `display` property. If needed, an internal cache can be easily added later.

* A number of APIs had the default values calculated in the directives instead of the `StyleBuilder`, meaning that it would be much harder to override by end-users. This has been fixed
* An issue where `fxLayoutGap` was canceling out `fxFlexOffset` has been corrected

* `MediaMonitor` (use `MediaMarshaller`)
* `ResponsiveActivation`
* `BreakpointX`
* `KeyOptions`
* `negativeOf`
* `BaseDirective` (use `BaseDirective2`)
* `BaseAdapter`

The minified size of the Angular Layout library has decreased *~38%* from **132KB** to **82KB**, a total savings of 50KB.  After the deprecated APIs are deleted (Beta.21), the build size will be reduced yet again.

It should also bring about performance improvements as a result of fewer RxJS subscriptions, memoized style lookups, and other API processing.

It is our hope that along with the added `StyleBuilder` functionality, and migration away from `ObservableMedia`, this represents the end-stage towards stability for Angular Layout.

Closes #903
Fixes #692
CaerusKaru added a commit that referenced this issue Dec 14, 2018
This design change marks a number of departures from the current Angular Layout configuration:

1. A number of APIs are deprecated in favor of a more polished, integrated API
2. A new semantics for creating custom breakpoint directives is introduced
3. A number of bugs caught along the way and design changes missed in past PRs are rectified

>  **PLEASE NOTE**: There will be **no end-user API changes** as a result of this PR.

Unless custom breakpoints are configured, devs should see no change in how the library usage. These changes will deliver notice significant size & performance improvements.

* `MediaMarshaller`
        A centralized data store for elements, breakpoints, and key-value pairs. The `MediaMarshaller` responds to mediaQuery changes and triggers appropriate Layout directives. This class also introduces a way to track changes for arbitrary elements and directive types
* `BaseDirective2`
        A new directive with stripped-down functionality from the old `BaseDirective` that is designed to work in symbiosis with the `MediaMarshaller`

The custom breakpoints story has changed significantly. Instead of extending directives that contain the default breakpoints and writing lengthy constructors, the process has been paired down to the following (i.e. for `fxFlexOffset`):

```ts
const inputs = ['fxFlexOffset.xss'];
const selector = `[fxFlexOffset.xss]`;

@directive({selector, inputs})
export class XssFlexOffsetDirective extends FlexOffsetDirective {
        protected inputs = inputs;
        }
        ```

Never again will a change in the base directive constructor require an entire rewrite of custom breakpoint directives. And registering a new directive no longer brings collisions and double-registrations competing with the default directives. Everything is separated and improved.

* All of the Grid and extended Flex directives have been updated to the new standards; namely, they have been refactored to the new API with `StyleBuilder`s. The notable exception is `show-hide`, which uses a `StyleBuilder`, but does not have a cache for the results.
> FxShowHide does not use a stylebuilder cache as to avoid complexity for cache-shifting based on the host `display` property. If needed, an internal cache can be easily added later.

* A number of APIs had the default values calculated in the directives instead of the `StyleBuilder`, meaning that it would be much harder to override by end-users. This has been fixed
* An issue where `fxLayoutGap` was canceling out `fxFlexOffset` has been corrected

* `MediaMonitor` (use `MediaMarshaller`)
* `ResponsiveActivation`
* `BreakpointX`
* `KeyOptions`
* `negativeOf`
* `BaseDirective` (use `BaseDirective2`)
* `BaseAdapter`

The minified size of the Angular Layout library has decreased *~38%* from **132KB** to **82KB**, a total savings of 50KB.  After the deprecated APIs are deleted (Beta.21), the build size will be reduced yet again.

It should also bring about performance improvements as a result of fewer RxJS subscriptions, memoized style lookups, and other API processing.

It is our hope that along with the added `StyleBuilder` functionality, and migration away from `ObservableMedia`, this represents the end-stage towards stability for Angular Layout.

Closes #903
Fixes #692
CaerusKaru added a commit that referenced this issue Dec 14, 2018
This design change marks a number of departures from the current Angular Layout configuration:

1. A number of APIs are deprecated in favor of a more polished, integrated API
2. A new semantics for creating custom breakpoint directives is introduced
3. A number of bugs caught along the way and design changes missed in past PRs are rectified

>  **PLEASE NOTE**: There will be **no end-user API changes** as a result of this PR.

Unless custom breakpoints are configured, devs should see no change in how the library usage. These changes will deliver notice significant size & performance improvements.

* `MediaMarshaller`
        A centralized data store for elements, breakpoints, and key-value pairs. The `MediaMarshaller` responds to mediaQuery changes and triggers appropriate Layout directives. This class also introduces a way to track changes for arbitrary elements and directive types
* `BaseDirective2`
        A new directive with stripped-down functionality from the old `BaseDirective` that is designed to work in symbiosis with the `MediaMarshaller`

The custom breakpoints story has changed significantly. Instead of extending directives that contain the default breakpoints and writing lengthy constructors, the process has been paired down to the following (i.e. for `fxFlexOffset`):

```ts
const inputs = ['fxFlexOffset.xss'];
const selector = `[fxFlexOffset.xss]`;

@directive({selector, inputs})
export class XssFlexOffsetDirective extends FlexOffsetDirective {
        protected inputs = inputs;
        }
        ```

Never again will a change in the base directive constructor require an entire rewrite of custom breakpoint directives. And registering a new directive no longer brings collisions and double-registrations competing with the default directives. Everything is separated and improved.

* All of the Grid and extended Flex directives have been updated to the new standards; namely, they have been refactored to the new API with `StyleBuilder`s. The notable exception is `show-hide`, which uses a `StyleBuilder`, but does not have a cache for the results.
> FxShowHide does not use a stylebuilder cache as to avoid complexity for cache-shifting based on the host `display` property. If needed, an internal cache can be easily added later.

* A number of APIs had the default values calculated in the directives instead of the `StyleBuilder`, meaning that it would be much harder to override by end-users. This has been fixed
* An issue where `fxLayoutGap` was canceling out `fxFlexOffset` has been corrected

* `MediaMonitor` (use `MediaMarshaller`)
* `ResponsiveActivation`
* `BreakpointX`
* `KeyOptions`
* `negativeOf`
* `BaseDirective` (use `BaseDirective2`)
* `BaseAdapter`

The minified size of the Angular Layout library has decreased *~38%* from **132KB** to **82KB**, a total savings of 50KB.  After the deprecated APIs are deleted (Beta.21), the build size will be reduced yet again.

It should also bring about performance improvements as a result of fewer RxJS subscriptions, memoized style lookups, and other API processing.

It is our hope that along with the added `StyleBuilder` functionality, and migration away from `ObservableMedia`, this represents the end-stage towards stability for Angular Layout.

Closes #903
Fixes #692
CaerusKaru added a commit that referenced this issue Dec 14, 2018
### Summary
This design change marks a number of departures from the current Angular Layout configuration:

1. A number of APIs are deprecated in favor of a more polished, integrated API
2. A new semantics for creating custom breakpoint directives is introduced
3. A number of bugs caught along the way and design changes missed in past PRs are rectified

>  **PLEASE NOTE**: There will be **no end-user API changes** as a result of this PR. 

Unless custom breakpoints are configured, devs should see no change in how the library usage. These changes will deliver significant size & performance improvements.

### New APIs

* `MediaMarshaller`
	A centralized data store for elements, breakpoints, and key-value pairs. The `MediaMarshaller` responds to mediaQuery changes and triggers appropriate Layout directives. This class also introduces a way to track changes for arbitrary elements and directive types
* `BaseDirective2`
	A new directive with stripped-down functionality from the old `BaseDirective` that is designed to work in symbiosis with the `MediaMarshaller`

### Custom Breakpoints

The custom breakpoints story has changed significantly. Instead of extending directives that contain the default breakpoints and writing lengthy constructors, the process has been pared down to the following (i.e. for `fxFlexOffset`):

```ts
const inputs = ['fxFlexOffset.xss'];
const selector = `[fxFlexOffset.xss]`;

@directive({selector, inputs})
export class XssFlexOffsetDirective extends FlexOffsetDirective {
	protected inputs = inputs;
}
```

Never again will a change in the base directive constructor require an entire rewrite of custom breakpoint directives. And registering a new directive no longer brings collisions and double-registrations competing with the default directives. Everything is separated and improved.

### Features

* All of the Grid and extended Flex directives have been updated to the new standards; namely, they have been refactored to the new API with `StyleBuilder`s. The notable exception is `show-hide`, which uses a `StyleBuilder`, but does not have a cache for the results. 
> FxShowHide does not use a StyleBuilder cache as to avoid complexity for cache-shifting based on the host `display` property. If needed, an internal cache can be easily added later.

### Bug Fixes

* A number of APIs had the default values calculated in the directives instead of the `StyleBuilder`, meaning that it would be much harder to override by end-users. This has been fixed
* An issue where `fxLayoutGap` was canceling out `fxFlexOffset` has been corrected
* The `ObservableMediaProvider` has been reintroduced to allow users to continue using `ObservableMedia` while it's deprecated 

### Deprecated APIs

* `MediaMonitor` (use `MediaMarshaller`)
* `ResponsiveActivation`
* `BreakpointX`
* `KeyOptions`
* `negativeOf`
* `BaseDirective` (use `BaseDirective2`)
* `BaseAdapter`

### Build Improvements

The minified size of the Angular Layout library has decreased *~27%* from **132KB** to **97KB**, a total savings of 35KB.  After the deprecated APIs are deleted (Beta.21), the build size will be reduced by another 15KB to 82KB, representing a total decrease of *~38%*.

### Performance Improvements

This commit should also bring about performance improvements as a result of fewer RxJS subscriptions, memoized style lookups, and other API processing. 

### Stabilization 

It is our hope that along with the added `StyleBuilder` functionality, and migration away from `ObservableMedia`, this represents the end-stage towards stability for Angular Layout.

Closes #903 
Fixes #692
@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
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug discussion Further discussion with the team is needed before proceeding has pr A PR has been created to address this issue P1 Urgent issue that should be resolved before the next re-lease
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants