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

Fix for DOM access before document ready #62

Merged
merged 8 commits into from
Jul 23, 2020
Merged

Fix for DOM access before document ready #62

merged 8 commits into from
Jul 23, 2020

Conversation

jrvidal
Copy link
Contributor

@jrvidal jrvidal commented Jul 21, 2020

I know this has been discussed previously as a WONTFIX, since good practices mandate that we load JS code at the bottom of the body element. However:

  • We currently don't have control over this in the CMS frontend.
  • Fixing it is going to be relatively costly (specially compared to the line count in this PR!)
  • We totally want to fix it properly 🤞, we just don't want to be blocked on new features while the work to fix it is ongoing.
  • I don't think there are significant downsides to this approach.
    • In particular, I don't think this will encourage people to load JS in the non-recommended way.
  • And, personally, I would argue that a library should make as few assumptions as possible about how it is loaded 🔥

@jrvidal jrvidal requested review from atabel and pladaria July 21, 2020 15:43
src/portal.tsx Outdated Show resolved Hide resolved
@jrvidal
Copy link
Contributor Author

jrvidal commented Jul 21, 2020

cc @ivmos

Copy link
Member

@pladaria pladaria left a comment

Choose a reason for hiding this comment

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

👍

src/portal.tsx Outdated Show resolved Hide resolved
@pladaria
Copy link
Member

Thanks! :)

@github-actions
Copy link

Deploy preview for mistica-web ready!

Built with commit f01a97a

✅ Preview: https://mistica-web-jyoape78i.vercel.app

This pull request is being automatically deployed with vercel-action

@github-actions
Copy link

Deploy preview for mistica-web ready!

Built with commit 8ef7ef0

✅ Preview: https://mistica-web-r4rub4z46.vercel.app

This pull request is being automatically deployed with vercel-action

@github-actions
Copy link

Deploy preview for mistica-web ready!

Built with commit 19100a5

✅ Preview: https://mistica-web-coopt2yy0.vercel.app

This pull request is being automatically deployed with vercel-action

src/portal.tsx Outdated Show resolved Hide resolved
@github-actions
Copy link

Deploy preview for mistica-web ready!

Built with commit d746a67

✅ Preview: https://mistica-web-mexw4c1er.vercel.app

This pull request is being automatically deployed with vercel-action

src/portal.tsx Outdated Show resolved Hide resolved
src/portal.tsx Outdated Show resolved Hide resolved
src/portal.tsx Outdated Show resolved Hide resolved
@jrvidal
Copy link
Contributor Author

jrvidal commented Jul 23, 2020

Phew, sorry this took way too many iterations.

src/portal.tsx Outdated Show resolved Hide resolved
src/portal.tsx Outdated Show resolved Hide resolved
src/portal.tsx Show resolved Hide resolved
@jrvidal
Copy link
Contributor Author

jrvidal commented Jul 23, 2020

Hm, not sure if you'd rather have me rebase at this point 😅

Copy link
Contributor

@atabel atabel left a comment

Choose a reason for hiding this comment

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

lgtm now 👍

@jrvidal jrvidal merged commit 3b5633e into master Jul 23, 2020
@pladaria pladaria deleted the portal-fix branch April 14, 2021 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants