fix(modal): adjust modal background to avoid shifting #2508

ymeine · 2018-07-09T15:54:54Z

Shifting potentially occurs when a scrollbar present on the container disappears once the modal opens.

The solution is taken from Bootstrap itself.

Tests are missing but require an e2e testing application.

fixes #641

maxokorokov

LGTM, a couple of minor comments.

It's a shame to do a couple of style recalcs and a layout, but it's what bootstrap does indeed. Can't find a better solution either.

maxokorokov · 2018-07-12T14:56:12Z

src/util/scrollbar.ts

@@ -0,0 +1,46 @@
+import {Injectable, Inject} from '@angular/core';
+import {DOCUMENT} from '@angular/platform-browser';


@angular/common, this one should be deprecated

maxokorokov · 2018-07-12T14:56:29Z

src/util/scrollbar.ts

+import {Injectable, Inject} from '@angular/core';
+import {DOCUMENT} from '@angular/platform-browser';
+
+import {isDefined} from './util';


maxokorokov · 2018-07-12T14:57:12Z

src/util/scrollbar.ts

+    return this.adjustBody(width);
+  }
+
+  adjustBody(width: number) {


adjustBody, isPresent, getWidth → private _adjustBody, private _isPresent, private _getWidth

maxokorokov · 2018-07-17T12:38:39Z

src/util/scrollbar.ts

+  private _adjustBody(width: number) {
+    const {body} = this._document;
+    const userSetPadding = body.style.paddingRight;
+    const paddingAmount = parseFloat(window.getComputedStyle(body)['padding-right']);


Hmm, I was just fixing SSR for the carousel and realised that window might break it too...

Regardless, I don't think the fix for this should be a part of this PR

pkozlowski-opensource · 2018-07-20T08:06:54Z

src/modal/modal-stack.ts

@@ -45,6 +46,7 @@ export class NgbModalStack {
    let windowCmptRef: ComponentRef<NgbModalWindow> = this._attachWindowComponent(containerEl, contentRef);
    let ngbModalRef: NgbModalRef = new NgbModalRef(windowCmptRef, contentRef, backdropCmptRef, options.beforeDismiss);

+    ngbModalRef.result.then(revertPaddingForScrollBar, revertPaddingForScrollBar);


Maybe write as ngbModalRef.result.finally(revertPaddingForScrollBar) ?

Forget it, it seems like browser support is still lacking... https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally

pkozlowski-opensource · 2018-07-20T08:09:52Z

src/util/scrollbar.ts

+export class ScrollBar {
+  constructor(@Inject(DOCUMENT) private _document) {}
+
+  compensate() {


Could you please add explicit return type so it is easier to read? We've got a return statement in this function but it is not immediately clear what is returned.

Yes, I'll add it along with documentation about it

pkozlowski-opensource · 2018-07-20T08:11:11Z

src/util/scrollbar.ts

+
+
+@Injectable()
+export class ScrollBar {


Could you add a bit of JS doc to this class with a brief description on how it works, what it can be used for etc.?

Yes, since I finally made it as a separate utility it makes sense

pkozlowski-opensource · 2018-07-20T08:12:13Z

src/util/scrollbar.ts

+  }
+
+  private _adjustBody(width: number) {
+    const {body} = this._document;


Wouldn't explicit const body = this._document.body; be more readable here?

Same on line 33, if you want to do something with it. But it's just a question of style to me...

Well, destructuring is standard feature, used elsewhere in this project, and avoids repetition. Then I could say it makes more sense when destructuring pure data objects (like parameters). I don't know 😛

pkozlowski-opensource · 2018-07-20T08:24:43Z

src/util/scrollbar.ts

+    return rect.left + rect.right < window.innerWidth;
+  }
+
+  private _getWidth(): number {


This could be called only once, if I'm not mistaken? (I mean, we don't have to call it in each and every compensate() call.

I don't know... could there be any situation where the width of the scrollbar would change (with custom styling then) throughout the application lifetime? (and would we support that?)

If not, I can cache the result.

Let's leave it as-is for now.

pkozlowski-opensource · 2018-07-20T08:25:21Z

src/util/scrollbar.ts

+
+  compensate() {
+    if (!this._isPresent()) {
+      return () => {};


nit: this is always the same, you could extract it as const noop = () => {};

Sure I will do

pkozlowski-opensource · 2018-07-20T08:26:39Z

@ymeine thnx for this PR, it fixes a long-lasting bug!

I've added few comments in the code - I would mostly like to see more docs (or at least types!).

maxokorokov · 2018-07-20T08:30:04Z

src/util/scrollbar.ts

+    const document = this._document;
+    const {body} = document;
+
+    const measurer = document.createElement('div');


By the way just realised that this is a lot more confusing to me. Unclear if it's a global document (wrong) or a local one.

(nitpicking, feel free to ignore)

Yeah... again that's a matter of style. And here the context is not far to check. I made like that to avoid repetition, but also to write in a more familiar style - which is exactly what is making you confused ^^

If you ask me I would:

drop the document const (and use this._document everywhere)

have const documentBody = this._document.createElement('div');

I see the word 'document' 4 times in the snippet above, so I won't agree with the repetition argument :)

vs

const body = this._document.body; // or const {body} = this._document; const measurer = this._document.createElement('div')

I can read your version fine too though

I was referring to repetition of property access mainly. But anyways this case is too small to have real benefit. I'll make the change.

Shifting potentially occurs when a scrollbar present on the container disappears once the modal opens. The solution is taken from Bootstrap itself. Remaining: - tests

Shifting potentially occurs when a scrollbar present on the container disappears once the modal opens. The solution is taken from Bootstrap itself. Fixes #641 Closes #2508

maxokorokov added component: modal needs: tests needs: review labels Jul 10, 2018

ymeine force-pushed the fix/641-modal-bgcontent-shifting branch from 0823f0f to 1cb52d8 Compare July 10, 2018 15:04

maxokorokov requested changes Jul 12, 2018

View reviewed changes

maxokorokov added needs: work and removed needs: review needs: tests labels Jul 12, 2018

ymeine force-pushed the fix/641-modal-bgcontent-shifting branch from 1cb52d8 to e5f0da1 Compare July 13, 2018 15:13

maxokorokov reviewed Jul 17, 2018

View reviewed changes

maxokorokov removed the needs: work label Jul 19, 2018

maxokorokov added this to the 3.0.0 milestone Jul 19, 2018

maxokorokov approved these changes Jul 19, 2018

View reviewed changes

pkozlowski-opensource reviewed Jul 20, 2018

View reviewed changes

maxokorokov reviewed Jul 20, 2018

View reviewed changes

fix(modal): adjust modal background to avoid shifting

4cbea01

Shifting potentially occurs when a scrollbar present on the container disappears once the modal opens. The solution is taken from Bootstrap itself. Remaining: - tests

ymeine force-pushed the fix/641-modal-bgcontent-shifting branch from e5f0da1 to 4cbea01 Compare July 20, 2018 13:52

pkozlowski-opensource closed this in 2871316 Jul 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(modal): adjust modal background to avoid shifting #2508

fix(modal): adjust modal background to avoid shifting #2508

ymeine commented Jul 9, 2018 •

edited

Loading

maxokorokov left a comment

maxokorokov Jul 12, 2018

maxokorokov Jul 12, 2018

maxokorokov Jul 12, 2018

maxokorokov Jul 17, 2018

maxokorokov Jul 17, 2018

pkozlowski-opensource Jul 20, 2018

pkozlowski-opensource Jul 20, 2018

pkozlowski-opensource Jul 20, 2018

ymeine Jul 20, 2018

pkozlowski-opensource Jul 20, 2018

ymeine Jul 20, 2018

pkozlowski-opensource Jul 20, 2018

maxokorokov Jul 20, 2018

ymeine Jul 20, 2018

pkozlowski-opensource Jul 20, 2018

ymeine Jul 20, 2018

pkozlowski-opensource Jul 20, 2018

pkozlowski-opensource Jul 20, 2018

ymeine Jul 20, 2018

pkozlowski-opensource commented Jul 20, 2018

maxokorokov Jul 20, 2018

ymeine Jul 20, 2018

pkozlowski-opensource Jul 20, 2018

maxokorokov Jul 20, 2018

ymeine Jul 20, 2018

		@@ -0,0 +1,46 @@
		import {Injectable, Inject} from '@angular/core';
		import {DOCUMENT} from '@angular/platform-browser';

fix(modal): adjust modal background to avoid shifting #2508

fix(modal): adjust modal background to avoid shifting #2508

Conversation

ymeine commented Jul 9, 2018 • edited Loading

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

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

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

pkozlowski-opensource commented Jul 20, 2018

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

ymeine commented Jul 9, 2018 •

edited

Loading