feat(overlay): more flexible scroll strategy API and ability to define/override custom strategies

[crisbeto]

• Refactors the overlay setup to allow for scroll strategies to be passed in by name, instead of by instance.
• Handles the scroll strategy dependency injection automatically.
• Adds an API for registering custom scroll strategies and overriding the existing ones.
• Adds a second parameter to the attach method, allowing for a config object to be passed in.
• Throws an error if there's an attempt to attach a scroll strategy multiple times. This is mostly a sanity check to ensure that we don't cache the scroll strategy instances.

Relates to #4093.

[jelbourn]

One comment that may be relevant even w/ changing the approach

[jelbourn]

ReflectiveInjector is meant to be JIT-only and will be going away soon.

[crisbeto]

@jelbourn updated the approach as discussed.

[jelbourn]

This way is still string-based; you can't get IDE completion or correction for the strategies available, and they can't be minified.

What I had been thinking was having each of these be properties. What's the value for those properties? Doing something like this would work....

export type ScrollStrategyOption = () => ScrollStrategy;

export class ScrollStrategyOptions {
  // The class symbol acts as a unique token for the strategy option.
  noop: ScrollStrategyOption = () => new NoopScrollStrategy();
  close: ScrollStrategyOption = () => new CloseScrollStrategy(this._scrollDispatcher);
  block: ScrollStrategyOption = () => new BlockScrollStrategy(this._viewportRuler);
  reposition: ScrollStrategyOption = () => new RepositionScrollStrategy(this._scrollDispatcher);

  getStrategyInstance(option: ScrollStrategyOption) {
    return option();
  }
}


// When using the options
overlayState.scrollStrategy = overlay.scrollStratgies.block;

I originally had them as the classes (BlockScrollStrategy) with a switch, but realized you could cut out that step completely if you treat the properties as the map to the creation directly. I like this approach because, to the user, each option is some opaque thing called ScrollStrategyOption, but on the inside we know it's a factory function.

When someone wants to customize the strategies, they would do this:

export class SuperScrollStrategyOptions extends ScrollStrategyOptions {
  // overwrite block strategy
  block: ScrollStrategyOption = () => new SuperBlockScrollStrategy(/* ... */);

  // define a custom strategy
  explode: ScrollStrategyOption = () => new SuperExploseScrollStrategy(/* ... */);
}

[crisbeto]

Nice, that's even better. I've updated the PR.

[jelbourn]

Expose scrollStrategies as a public property on Overlay so each component won't have to inject it separately?

[jelbourn]

I just realized that we lose out on having config types specific to the strategy with this approach. So......

What if we tweak the options setup slightly so that it's something like...

// in scroll-strategy-option.ts
export interface ScrollStrategyOption {
  _create: () => ScrollStrategy;
}

// in block-scroll-strategy.ts
export class BlockScrollStrategyOption {
  _config = new BlockScrollStrategyConfig();
  _create() {
    return new BlockScrollStrategy(..., config)
  }

  withExtraOption(value) {
    this._config.extraOption = value;
    return this;
  }
}

// in scroll-strategy-options.ts
export class ScrollStrategyOptions {
  constructor(private _scrollDispatcher: ScrollDispatcher, private _viewportRuler: ViewportRuler) { }

  noop: ScrollStrategyOption = basicOption(() => new NoopScrollStrategy());
  close: ScrollStrategyOption = basicOption(() => new CloseScrollStrategy(this._scrollDispatcher));
  block = new BlockScrollStrategyOption(...);
  reposition: ScrollStrategyOption = basicOption(() => new RepositionScrollStrategy(this._scrollDispatcher));

  basicOption(factoryFn): ScrollStrategyOption {
    return {_create: factoryFn};
  }
}

For someone just consuming the options, this is identical but with a nicer interface for the configuration. For people creating a custom strategy, they just have to create this new builder option class, which I think is worth it when capturing the configuration.

Sorry for the churn- I keep thinking of new things.

[crisbeto]

@jelbourn added the discussed changes. It cut down ~100 LoC.

[jelbourn]

Looks good, just a few last comments

[jelbourn]

Omit the return?

  reposition = (config?: RepositionScrollStrategyConfig) =>
      new RepositionScrollStrategy(this._scrollDispatcher, config);

[jelbourn]

How about

/**
 * Options for how an overlay will handle scrolling.
 *
 * Users can provide a custom value for `ScrollStrategyOptions` to replace the default
 * behaviors. This class primarily acts as a factory for ScrollStrategy instances.
 */

[jelbourn]

We should add a public JsDoc for each for these, e.g.

/** Do nothing on scroll. */

/** Close the overlay on scroll. */

[jelbourn]

quirk: the new isn't actually necessary for Error

[crisbeto]

Feedback has been addressed.

[jelbourn]

LGTM

[angular-automatic-lock-bot]

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