fix: Compute scrollbar size dynamically on need

[billychan]

About issue #283 and react bootstrap issue react-bootstrap/react-bootstrap#3645

Verified locally react-bootstrap modal launched well.

[taion]

Please see discussion around #283 (comment). This is not an acceptable fix. Calculating the scrollbar size requires inserting new elements to the DOM, and should not be done during the setContainerStyle calculation.

[billychan]

@taion you are right at that point. How do you like if the manager instance instantiated here? https://github.com/billychan/react-overlays/blob/c885aa3102ec01569d18df447f67cb2f3a950eb9/src/Modal.js#L235

[taion]

Yes – I think componentDidMount is the right place to construct the default modal manager.

[billychan]

@taion Thanks checking man, please see how the new version looks 😃

[taion]

• You should only instantiate this if a manager wasn't specified via props.
• By default, we still want a single global singleton manager. We just want to delay when it gets initialized.

So I think I'd do something like:

let manager; // Maybe export this?

class Modal {
  // ...

  getModalManager() {
    if (this.props.manager) {
      return this.props.manager;
    }

    if (!manager) {
      manager = new ModalManager();
    }

    return manager;
  }

  // ...
}

[billychan]

My bad, I previously wrote that as this.manager = this.props.manager || new ModalManager() to still allow injection from props, removed when testing some issue and forgot to revert. Thanks catching 👍

[taion]

But even so that doesn't quite preserve the semantics, as it means that changing the passed-in modal manager via the props would have no effect.

[billychan]

I saw your point, you want to keep a singleton of ModalManager instance which keeps global knowledge of this.modals.

That might be better wrapped in a separate module I guess, maybe something like this?

// Modal.js
import { getModalManager } from './ModalManager';

//.....
  componentDidMount() {
    this._isMounted = true;
    this.manager = this.props.manager || getModalManager();

And

// ModalManager.js
// ....

let manager;

function getModalManager() {
  if (manager) {
     return manager;
  }
 manager = new ModalManager();
 return manager;
}

Sounds good?

BTW maybe similar needs to be done in react-bootstrap later too https://github.com/react-bootstrap/react-bootstrap/blob/0bb8a6fd4ad110c49e5e89f05228ade0968652bf/src/Modal.js#L154

[taion]

See the example code in #297 (comment). Do that, then use this.getModalManager(). The issue with your proposed solution above is that it wouldn't respect changes to the prop.

[deXterbed]

@taion any update on when this PR will be merged? thanks!

[taion]

getModalManager doesn't need to be bound, as it's always called with the proper this context

additionally, you may need to merge to master to get CI to pass

i'll merge afterward. thanks!

[taion]

Suggested change

	getModalManager = () => {
	getModalManager() {

[billychan]

right 👍

[taion]

Suggested change

	};
	}

[deXterbed]

@billychan can you look into the changes suggested by taion?

[billychan]

@deXterbed @taion I've merged back master and updated code to remove binding.

The specs all passed while CI failed at uploading tests coverage step, maybe that's something temporary I guess.

[taion]

Thanks!