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

feat(core): add centralized media marshal service #900

Merged
merged 1 commit into from
Dec 14, 2018
Merged

Conversation

CaerusKaru
Copy link
Member

@CaerusKaru CaerusKaru commented Dec 4, 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 notice 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 paired down to the following (i.e. for fxFlexOffset):

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 StyleBuilders. 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

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 ~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.

Performance Improvements

It 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

}
}

destroy(): void {
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be fleshed out due to the subscribe-y nature of this, but also the ephemeral nature of services.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should talk about this one.

@CaerusKaru
Copy link
Member Author

CaerusKaru commented Dec 4, 2018

Still needs the following after design sign-off:

  • Unit tests for MediaMarshaller
  • New name and deprecation strategy for NewBaseDirective and BaseDirective
  • Move ngOnChanges into NewBaseDirective?
  • Come up with a new strategy for storing the flex value in the marshaller

@CaerusKaru
Copy link
Member Author

The thought occurs to me that we could still support activatedValue on NewBaseDirective instead of removing it:

get activatedInput(): string {
	return this.marshal.getValue(this.nativeElement, this.DIRECTIVE_KEY);
}
set activatedInput(value: string) {
	this.marshal.setValue(this.nativeElement, this.DIRECTIVE_KEY, value, this.marshal.activatedBreakpoint);
}

@CaerusKaru
Copy link
Member Author

NOTE: there is a critical issue with caching and directionality right now affecting fxFlex

@CaerusKaru
Copy link
Member Author

Issue has been resolved, this PR is now ready for review 👍

Copy link
Contributor

@ThomasBurleson ThomasBurleson left a comment

Choose a reason for hiding this comment

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

Love these changes. Only a few suggestions and questions.

src/lib/core/base/new-base.ts Outdated Show resolved Hide resolved
src/apps/universal-app/src/app/split/split.directive.ts Outdated Show resolved Hide resolved
src/lib/core/base/index.ts Outdated Show resolved Hide resolved
src/lib/core/breakpoints/break-point.ts Outdated Show resolved Hide resolved
src/lib/extended/class/class.ts Show resolved Hide resolved
src/lib/extended/module.ts Show resolved Hide resolved
src/lib/extended/show-hide/show.spec.ts Show resolved Hide resolved
src/lib/flex/flex/flex.ts Outdated Show resolved Hide resolved
src/lib/flex/layout-gap/layout-gap.ts Show resolved Hide resolved
@CaerusKaru CaerusKaru force-pushed the adam/marshal branch 2 times, most recently from 24534dc to b7d26ab Compare December 13, 2018 03:10
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
@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes custom breakpoints Issues with Custom Breakpoint configuration and use feature performance pr: needs review
Projects
None yet
4 participants