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: add option for prepending (no)script to body #410

Merged
merged 7 commits into from
Jul 17, 2019
Merged

Conversation

pimlie
Copy link
Collaborator

@pimlie pimlie commented Jul 16, 2019

Resolves: #370

I couldnt came up with a good single name, so I decided on pody as in prepended body.

I am happy to change this to something better, but I'd prefer to keep it to a single word so we dont have kebab/camelCase issues when switching between js object property and html attribute name.

@pimlie pimlie requested review from atinux and manniL July 16, 2019 15:17
@atinux
Copy link
Member

atinux commented Jul 16, 2019

Nice PR @pimlie

I don't know what to feel about pody, I would have mabe called it pbody to keep body and p for prepend.

@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #410 into master will decrease coverage by 0.44%.
The diff coverage is 95.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
- Coverage     100%   99.55%   -0.45%     
==========================================
  Files          30       31       +1     
  Lines         430      449      +19     
  Branches      125      127       +2     
==========================================
+ Hits          430      447      +17     
- Misses          0        2       +2
Impacted Files Coverage Δ
src/client/updateClientMetaInfo.js 100% <ø> (ø) ⬆️
src/shared/constants.js 100% <100%> (ø) ⬆️
src/utils/elements.js 100% <100%> (ø)
src/server/generators/tag.js 100% <100%> (ø) ⬆️
src/client/updaters/tag.js 96.07% <93.75%> (-3.93%) ⬇️

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 f90cd41...156d816. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #410 into master will decrease coverage by 0.44%.
The diff coverage is 95.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
- Coverage     100%   99.55%   -0.45%     
==========================================
  Files          30       31       +1     
  Lines         430      449      +19     
  Branches      125      127       +2     
==========================================
+ Hits          430      447      +17     
- Misses          0        2       +2
Impacted Files Coverage Δ
src/client/updateClientMetaInfo.js 100% <ø> (ø) ⬆️
src/shared/constants.js 100% <100%> (ø) ⬆️
src/utils/elements.js 100% <100%> (ø)
src/server/generators/tag.js 100% <100%> (ø) ⬆️
src/client/updaters/tag.js 96.07% <93.75%> (-3.93%) ⬇️

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 f90cd41...dae4469. Read the comment docs.

@pimlie
Copy link
Collaborator Author

pimlie commented Jul 17, 2019

Renamed pody to pbody

@atinux
Copy link
Member

atinux commented Jul 17, 2019

Good for me (expect failing test 😄 )

@pimlie
Copy link
Collaborator Author

pimlie commented Jul 17, 2019

Ha, thanks for the heads-up. Getting a bit too used to failing tests due to the IE issue I guess ;)

@pimlie pimlie merged commit 05163a7 into master Jul 17, 2019
@derz
Copy link

derz commented Jul 18, 2019

thanks for you work @pimlie (cc @atinux). Wouldn‘t it be nicer to extend the existing body flag and include two options for it - maybe: afterbegin and beforeend like it is used, for example, in here: https://developer.mozilla.org/de/docs/Web/API/Element/insertAdjacentHTML
We could use beforeend as the default value if it is set to true to avoid breaking changes and avoid a potential pitfall if both flags (body and pbody`) are set.

@pimlie
Copy link
Collaborator Author

pimlie commented Jul 22, 2019

@derz Thank you for your comment. I can see the logic in your suggestion. After this PR was merged I realized it would maybe also be nice to just add extra methods, so instead of calling text() you'd call meta.script.body() / meta.script.bodyAppend() vs meta.script.bodyPrepend()

But I am not strongly opinionated on the matter, so I will leave the initiative to change this API to someone else :)

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.

Feature Request: add scripts right after opening <body> tag
3 participants