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

Added 'async' optional param to 'regClientScript' and 'regClientStartupScript' #14527

Closed
wants to merge 2 commits into from
Closed

Added 'async' optional param to 'regClientScript' and 'regClientStartupScript' #14527

wants to merge 2 commits into from

Conversation

DettRoxx
Copy link
Contributor

@DettRoxx DettRoxx commented Apr 2, 2019

What does it do?

Added optional param 'async' for methods regClientScript and regClientStartupScript of modx object, which allows to get

<script type="text/javascript" src="/assets/js/script.js" async></script>
or
<script type="text/javascript" src="/assets/js/script.js" defer></script>

instead of
<script type="text/javascript" src="/assets/js/script.js"></script>

Why is it needed?

to avoid "blocking-like" synchronous behaviour when browser builds DOM and stops and execute scripts tags, allows to use asynchronous behaviour instead

Related issue(s)/PR(s)

#14520

@DettRoxx
Copy link
Contributor Author

DettRoxx commented Apr 3, 2019

Need to update UnitTests for this PR

* @return void
*/
public function regClientStartupScript($src, $plaintext= false) {
public function regClientStartupScript($src, $plaintext= false, $async='') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only two params allowed - async and defer, so we should add check if passed param is correct or just make it possible to turn it into a boolean parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. now params checks correct values

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add an attributes parameter. The script tag can also have other attributes like id.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are also other methods available (e.g. regClientHTML) so I don't think we need an option for every possibility here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about something like this:

public function regClientStartupScript($src, $plaintext= false, $attributes = [])

$attributes = ['async', 'defer', 'id' => ['test'], 'etc'];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but for what? i opened w3 specs for 'script' tag:
we see:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

You are referring to W3 schools. Not the w3 specs.
See this list of supported attributes.
This also includes all global attributes.

@Mark-H
Copy link
Collaborator

Mark-H commented Apr 5, 2019

This just needs the tests to be updated to get my approval.

@Mark-H Mark-H added this to the v3.0.0-alpha milestone Apr 5, 2019
@Mark-H Mark-H added the blocked Active participation around the pull request or issue required. Consensus is not reached. label Apr 5, 2019
Copy link
Collaborator

@Mark-H Mark-H left a comment

Choose a reason for hiding this comment

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

Failing tests need to be updated.

@JoshuaLuckers
Copy link
Contributor

@DettRoxx if you need any help fixing the test please let us know 👍

@Mark-H
Copy link
Collaborator

Mark-H commented Sep 1, 2019

Closing due to lack of interaction - if someone wants to pitch in to get this up to speed again that'd be awesome.

@Mark-H Mark-H closed this Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Active participation around the pull request or issue required. Consensus is not reached.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants