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(positionning): open popperOptions api #4323

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

fbasso
Copy link
Member

@fbasso fbasso commented May 4, 2022

First draft about how to give access to the popper options.

@fbasso fbasso marked this pull request as draft May 4, 2022 13:36
@fbasso fbasso requested a review from maxokorokov May 4, 2022 13:37
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #4323 (13abc84) into master (5122159) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4323      +/-   ##
==========================================
+ Coverage   89.11%   89.15%   +0.04%     
==========================================
  Files         113      113              
  Lines        3693     3708      +15     
  Branches      693      693              
==========================================
+ Hits         3291     3306      +15     
  Misses        357      357              
  Partials       45       45              
Flag Coverage Δ
e2e 50.08% <100.00%> (+0.17%) ⬆️
unit 88.88% <100.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
src/datepicker/datepicker-input-config.ts 100.00% <100.00%> (ø)
src/datepicker/datepicker-input.ts 94.03% <100.00%> (+0.08%) ⬆️
src/dropdown/dropdown-config.ts 100.00% <100.00%> (ø)
src/dropdown/dropdown.ts 96.51% <100.00%> (+0.03%) ⬆️
src/popover/popover-config.ts 81.81% <100.00%> (+1.81%) ⬆️
src/popover/popover.ts 96.05% <100.00%> (+0.10%) ⬆️
src/tooltip/tooltip-config.ts 81.81% <100.00%> (+1.81%) ⬆️
src/tooltip/tooltip.ts 98.46% <100.00%> (+0.04%) ⬆️
src/typeahead/typeahead-config.ts 100.00% <100.00%> (ø)
src/typeahead/typeahead.ts 97.74% <100.00%> (+0.03%) ⬆️
... and 1 more

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 5122159...13abc84. Read the comment docs.

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

Hey, left some comments!

@@ -17,5 +18,6 @@ export class NgbInputDatepickerConfig extends NgbDatepickerConfig {
container: null | 'body';
positionTarget: string | HTMLElement;
placement: PlacementArray = ['bottom-start', 'bottom-end', 'top-start', 'top-end'];
popperOptions: (options: Partial<Options>) => Partial<Options>;
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should initialize directly with function, not undefined?

@@ -289,6 +298,7 @@ export class NgbInputDatepicker implements OnChanges,
@Inject(DOCUMENT) private _document: any, private _changeDetector: ChangeDetectorRef,
config: NgbInputDatepickerConfig) {
['autoClose', 'container', 'positionTarget', 'placement'].forEach(input => this[input] = config[input]);
this.popperOptions = config.popperOptions || ((options: Partial<Options>) => options);
Copy link
Member

Choose a reason for hiding this comment

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

if we initialize directly in config, we should avoid having this

@@ -394,7 +404,9 @@ export class NgbInputDatepicker implements OnChanges,
targetElement: this._cRef.location.nativeElement,
placement: this.placement,
appendToBody: this.container === 'body',
updatePopperOptions: addPopperOffset([0, 2])
updatePopperOptions: (options) => {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: arrow function?

src/util/positioning.spec.ts Show resolved Hide resolved
const popover = fixture.debugElement.query(By.directive(NgbPopover)).injector.get(NgbPopover);

const spy = createSpy();
popover.popperOptions = (options: Partial<Options>) => {
Copy link
Member

@maxokorokov maxokorokov Aug 2, 2022

Choose a reason for hiding this comment

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

I wonder if we should explain how to use it more clearly in docs:

  • is the initial config readonly or can be modified?
  • should the user clone it?
  • will initial config be discarded and ignored?

@fbasso
Copy link
Member Author

fbasso commented Aug 2, 2022

Thanks for your review and comments, @maxokorokov !

I've changed the code accordingly.

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM!

@maxokorokov maxokorokov merged commit a6f6803 into ng-bootstrap:master Aug 2, 2022
@fbasso fbasso deleted the popper-config branch August 2, 2022 14:52
@maxokorokov maxokorokov linked an issue Aug 16, 2022 that may be closed by this pull request
@sergeyglazyrindev
Copy link

Hello guys. Could you please suggest how do I proceed to get it working in my project ? The fix is not published yet on npm, right ? So, shall I use packag directly from github ? When are you gonna publish this version on npm ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow customizing popper config
4 participants