feat(positioning): popper 2 #4129

fbasso · 2021-07-20T13:22:24Z

No description provided.

codecov-commenter · 2021-08-30T14:40:53Z

Codecov Report

Merging #4129 (739bf65) into bootstrap5 (9598506) will increase coverage by 0.15%.
The diff coverage is 98.00%.

@@              Coverage Diff               @@
##           bootstrap5    #4129      +/-   ##
==============================================
+ Coverage       87.82%   87.97%   +0.15%     
==============================================
  Files             113      113              
  Lines            3629     3551      -78     
  Branches          676      654      -22     
==============================================
- Hits             3187     3124      -63     
+ Misses            368      357      -11     
+ Partials           74       70       -4

Flag	Coverage Δ
e2e	`45.98% <94.00%> (-0.65%)`	⬇️
unit	`87.41% <97.87%> (+0.33%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/datepicker/datepicker-input.ts	`95.77% <ø> (-1.41%)`	⬇️
src/typeahead/typeahead.ts	`96.09% <ø> (ø)`
src/util/positioning.ts	`95.12% <97.05%> (+10.82%)`	⬆️
src/dropdown/dropdown.ts	`96.17% <100.00%> (+0.04%)`	⬆️
src/popover/popover.ts	`95.65% <100.00%> (ø)`
src/tooltip/tooltip.ts	`98.27% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9598506...739bf65. Read the comment docs.

maxokorokov

Hey @fbasso,

here is what I've found so far:

Big issues

Calling positionElements recreates popper every time is called. This is an issue for example in dropdown when we do it every time zone is stable. We should redesign the API for repositioning I guess and either create a new popper or do popper.update() when necessary.
I think we should change posisitonElements to accept popper config as an argument to avoid component-specific code in positioning.ts (ex. for popover offset 8px should be hardcoded) and allowing end-users to provide custom popper configs if they need (separate PR for the last one I guess).
Popper is destroyed correctly only in tooltip and popover

Minor issues

tooltip/popover arrow is centered correctly only AFTER the next CD, but not initially (might be related to the 1st major issue)
dropdown/datepicker start flipping randomly on CD (definitely related to the 1st major issue)
we should not use deep imports, but import everything form @popper/core
dropdown → we should check that keyboard works as expected and dropup/dropdown works right when popper flips menu positioning automatically
tests broken with latest popper 2.10 (e2e dropdown positioning)

Everything else is inline in code!

Cheers,
Max

maxokorokov · 2021-09-08T09:19:34Z

src/datepicker/datepicker-input.spec.ts

@@ -945,57 +944,6 @@ describe('NgbInputDatepicker', () => {

  describe('positionTarget', () => {

-    let positionElementsSpy: jasmine.Spy;


Removing this decreases coverage for positionTarget tests

maxokorokov · 2021-09-08T09:20:19Z

src/dropdown/dropdown.spec.ts

+       // open
+       dropdown.open();
+       fixture.detectChanges();
+       tick(25);


Seems like popper uses promises for scheduling, so 25 is not necessary, just tick()

maxokorokov · 2021-09-08T09:20:50Z

src/dropdown/dropdown.spec.ts

@@ -131,49 +131,52 @@ describe('ngb-dropdown', () => {
    expect(dropdownEl).toHaveCssClass('dropdown');
  });

-  it('should allow setting a custom dropdown class (container="body")', () => {
-    const html = `
+  it('should allow setting a custom dropdown class (container="body")', fakeAsync(() => {


If I'm not mistaken this specific test actually works fine without any changes

maxokorokov · 2021-09-08T09:21:11Z

src/dropdown/dropdown.ts

@@ -63,6 +63,7 @@ export class NgbDropdownItem {
    '[class.dropdown-menu]': 'true',
    '[class.show]': 'dropdown.isOpen()',
    '[attr.x-placement]': 'placement',


should remove x-placement I guess?

src/popover/popover.ts

maxokorokov · 2021-09-08T09:31:17Z

src/util/positioning.ts

-    return elOffset;
+const popperStartPrimaryPlacement = /^left/;
+const popperEndPrimaryPlacement = /^right/;
+function convertPlacementToBsClasses(baseClass: string, placement: PopperPlacement): string {


maybe something like getBootstrapBaseClassPlacement. I was struggling to understand what is does.

Also unit tests for these kind of utility functions would be nice...

maxokorokov · 2021-09-08T09:33:02Z

src/util/positioning.ts

+
+  const bsModifier: Partial<Modifier<any, any>> = {
+    name: 'bootstrapClasses',
+    enabled: true,


I think it should be enabled: !!baseClass to avoid executing it for components without the base class

maxokorokov · 2021-09-08T09:33:22Z

src/util/positioning.ts

-  }
+  let mainPlacement = popperPlacements.shift();
+
+  const bsModifier: Partial<Modifier<any, any>> = {


maybe bootstrapBaseClassModifier ?

maxokorokov · 2021-09-08T09:34:45Z

src/util/positioning.ts

+        enabled: true,
+        options: {
+          offset: () => {
+            return baseClass === 'bs-popover' ? [0, 12] : [];


This should be passed as a partial config only from calling positionElements from the popover...
We should not have component-specific code here. Also 8px, not 12 → https://github.com/twbs/bootstrap/blob/main/js/src/popover.js#L25

maxokorokov · 2021-09-08T09:35:00Z

src/util/positioning.ts

+        },
+      },
+      {
+        name: 'preventOverflow',


don't think this is necessary

fbasso · 2021-10-14T14:31:23Z

@maxokorokov, I've updated the PR to fix almost all the issues you've listed.

I've refactor the positioning service to provide a ngbPositioning function to manage the creation/update/destroy lifecycle, and to be able to change the popper config from outside (required for popper.js, and open future possibilities for user to interact directly with the popper options).

Some issues to be investigated yet: the arrow is still not well positioned on the tooltip, deep import not completely removed (defaultModifiers, OptionsGeneric not available on @popperjs/core) and "decreases coverage" still pending.

Anyway, I think there are now noticeable changes to review !

maxokorokov

Hey, @fbasso!

I did another round and left some comments.
Most issues are gone for me except for tooltip/popover arrow.
I think we should not move to services and DI at the moment and leave like this.

A couple of things:

this._positioning.update is called inconsistently across components (sometimes outside zone, sometimes inside, sometimes on stable, etc)
logs/comments in the code

Cheers,
Max

maxokorokov · 2021-10-19T09:22:18Z

src/util/positioning.ts

+  popperGenerator,
+  preventOverflow
+} from '@popperjs/core';
+import {defaultModifiers, OptionsGeneric} from '@popperjs/core/lib/popper-lite';


We should import things only from @popperjs/core and avoid deep imports like this.
Also as discussed previously we could use createPopperLite instead of popperGenerator

maxokorokov · 2021-10-19T09:23:37Z

src/util/positioning.ts

+  placement: string | Placement | PlacementArray;
+  appendToBody?: boolean;
+  baseClass?: string;
+  updatePopperOptions?: (options: Partial<OptionsGeneric<any>>) => Partial<OptionsGeneric<any>>;


Why are you using OptionsGeneric<any> everywhere instead of just Options ?

maxokorokov · 2021-10-19T09:25:34Z

src/util/positioning.ts

+      let popperOptions = updatePopperOptions(getPopperOptions(positioningOption));
+      if (popperInstance) {
+        popperInstance.setOptions(popperOptions);
+        popperInstance.update();


No need in .update() here, if you do setOptions. But on the other hand I'm not sure we should call setOptions on every update... I think we should have:

update() setOptions(options) destroy()

as the API ideally

maxokorokov · 2021-10-19T09:32:26Z

src/tooltip/tooltip.ts

-   * Possible values are `"top"`, `"top-start"`, `"top-end"`, `"bottom"`, `"bottom-start"`,
-   * `"bottom-end"`, `"start"`, `"start-top"`, `"start-bottom"`, `"end"`, `"end-top"`,
-   * `"end-bottom"`
+   * `"top"`, `"top-start"`, `"top-left"`, `"top-end"`, `"top-right"`,


You removed the beginning "Possible values are" text

fbasso · 2021-10-22T13:21:31Z

Thanks for all your comments, @maxokorokov !

This is a new push with a lot of (corresponding) changes. I hope I didn't forget anything 😃

Let me know what you think...

maxokorokov

LGTM apart from the popover.ts implementation.
update() should be called onStable and not only when opening the popup.

src/popover/popover.ts

BREAKING CHANGES: ng-bootstrap now has a dependency on `@popper/core`. All positioning logic for datepicker, dropdown, popover, tooltip and typeahead is outsourced to popper.js

fbasso force-pushed the popper2 branch 3 times, most recently from d547838 to ac931ac Compare July 27, 2021 14:11

fbasso force-pushed the popper2 branch from ac931ac to c6d0f53 Compare August 30, 2021 14:24

fbasso force-pushed the popper2 branch from c6d0f53 to 739bf65 Compare August 30, 2021 14:43

fbasso marked this pull request as ready for review August 30, 2021 15:04

fbasso requested a review from maxokorokov August 30, 2021 15:04

maxokorokov added breaking change needs: review type: feature type: positioning labels Sep 6, 2021

maxokorokov added this to the 11.0.0 milestone Sep 6, 2021

maxokorokov mentioned this pull request Sep 8, 2021

Add Popper 2 as a dependency #3579

Closed

14 tasks

maxokorokov requested changes Sep 8, 2021

View reviewed changes

maxokorokov added needs: design needs: work and removed needs: review labels Sep 8, 2021

donalfenwick mentioned this pull request Sep 17, 2021

fix(dropdown): fix for menu alignment issue mentioned in #4100 #4136

Closed

5 tasks

fbasso force-pushed the popper2 branch from 739bf65 to 6fd40c2 Compare October 14, 2021 14:10

fbasso force-pushed the popper2 branch 2 times, most recently from 6c809e7 to cc44491 Compare October 14, 2021 14:37

maxokorokov requested changes Oct 19, 2021

View reviewed changes

fbasso force-pushed the popper2 branch 3 times, most recently from 768124c to dea093d Compare October 22, 2021 13:19

fbasso added the bootstrap 5 label Oct 25, 2021

Zwergenpunk mentioned this pull request Oct 25, 2021

Bootstrap 5 support #3899

Closed

49 tasks

maxokorokov requested changes Oct 26, 2021

View reviewed changes

src/popover/popover.ts Outdated Show resolved Hide resolved

maxokorokov force-pushed the popper2 branch from dea093d to 9778f8c Compare October 26, 2021 12:51

feat(positioning): use popper 2 for positioning

f72a34f

BREAKING CHANGES: ng-bootstrap now has a dependency on `@popper/core`. All positioning logic for datepicker, dropdown, popover, tooltip and typeahead is outsourced to popper.js

maxokorokov force-pushed the popper2 branch from 9778f8c to f72a34f Compare October 26, 2021 13:17

maxokorokov merged commit d2ca848 into ng-bootstrap:bootstrap5 Oct 26, 2021

maxokorokov mentioned this pull request Nov 4, 2021

Is ng-bootstrap still under development? #4177

Closed

fbasso deleted the popper2 branch November 8, 2021 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(positioning): popper 2 #4129

feat(positioning): popper 2 #4129

fbasso commented Jul 20, 2021

codecov-commenter commented Aug 30, 2021 •

edited

maxokorokov left a comment •

edited

maxokorokov Sep 8, 2021

maxokorokov Sep 8, 2021

maxokorokov Sep 8, 2021

maxokorokov Sep 8, 2021

maxokorokov Sep 8, 2021

maxokorokov Sep 8, 2021

maxokorokov Sep 8, 2021

maxokorokov Sep 8, 2021

maxokorokov Sep 8, 2021

fbasso commented Oct 14, 2021

maxokorokov left a comment

maxokorokov Oct 19, 2021

maxokorokov Oct 19, 2021

maxokorokov Oct 19, 2021

maxokorokov Oct 19, 2021

fbasso commented Oct 22, 2021

maxokorokov left a comment

		@@ -945,57 +944,6 @@ describe('NgbInputDatepicker', () => {

		describe('positionTarget', () => {

		let positionElementsSpy: jasmine.Spy;

feat(positioning): popper 2 #4129

feat(positioning): popper 2 #4129

Conversation

fbasso commented Jul 20, 2021

codecov-commenter commented Aug 30, 2021 • edited

Codecov Report

maxokorokov left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbasso commented Oct 14, 2021

maxokorokov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbasso commented Oct 22, 2021

maxokorokov left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 30, 2021 •

edited

maxokorokov left a comment •

edited