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

harmonize code paths used in FastBoot and non-FastBoot of <BsModal> #1533

Merged
merged 2 commits into from
May 29, 2021

Conversation

jelhan
Copy link
Contributor

@jelhan jelhan commented May 27, 2021

Running same code paths in FastBoot as in "regular" environment reduces the risk of bugs. To do so I moved the guards for FastBoot as close as possible to the places in code, which access document or window. Tests are passing and I manually verified that /fastboot/modal URL of dummy app shows the modal even with JavaScript being disabled. Not sure if additional manual tests are required.

Another small refactoring pulled out of #1402.

Copy link
Contributor

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

As much as I loved the previous round of refactorings, I am actually not so sure about this tbh.

If the concern of a function is to do something which does not make any sense in a different context, I don't think that function should be responsible to do the check, rather it's the caller's responsibility.

That applies at least for the scrollbar stuff for example, where we are not doing something that creates markup like adding a class (which would also apply to FastBoot), but manipulation DOM properties. Static HTML markup (like a string of HTML, which FastBoot effectively generates) does not know about scrollbars, so why should scrollbar-related functions need to take these environments into account?

So idk, I don't feel super strong about this, just some mild concerns/questions...

@jelhan
Copy link
Contributor Author

jelhan commented May 28, 2021

Main driver of the refactoring was to run show method in handleVisibilityChanges and not skip it early. Mostly because that relies on modal being opened in a different way than through show method. Basically the coupling of internal and external modal state, which I tried to address in #1402.

Not sure if I have any opinion if setScrollbar should guards against FastBoot or if the caller should do so. Happy to refactor to something like this if you prefer:

async show() {
  // ...

  if (!isFastBoot(this)) {
    this.checkScrollbar();
    this.setScrollbar();
  }
  
  // ...
}

@simonihmig
Copy link
Contributor

Not a big deal as I said, but yes, I would slightly prefer that...

@jelhan jelhan merged commit 822bf2f into master May 29, 2021
@jelhan jelhan deleted the use-same-code-path-for-bs-modal-if-run-in-fastboot branch May 29, 2021 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants