Script Loading Strategy - Update standalone business logic #47
Conversation
…cement/update-wp-add-inline-script-standalone
kt-12
added
the
Script Loading Strategy
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
| $this->assertSame( $expected, $output, $message ); | ||
| } | ||
|
|
||
| public function data_standalone_inline_script() { |
There was a problem hiding this comment.
Missing docblock, I know it's a data provider for the test but we should still have docblocks or not?
There was a problem hiding this comment.
Same comment about the use of a data provider here.
There was a problem hiding this comment.
As mentioned above this will be handled in PR #50.
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>
There was a problem hiding this comment.
The functionality here looks good, I left some feedback about the use of data providers in this context, which I think we should adjust either here or in a following PR if needed.
To address your questions about adding doc blocks, I think that even though there are several places in the current WordPress unit tests where those don't exist, I would prefer us put in the extra effort to put some doc blocks in place on data providers wherever we are using them.
| $this->assertSame( $expected, $output, $message ); | ||
| } | ||
|
|
||
| public function data_non_standalone_before_inline_script_with_defer() { |
There was a problem hiding this comment.
Generally speaking, data providers are meant to provide inputs into whatever functionality you are unit testing, and not just passing parameters to a PHPUnit assertion. I think that these would probably be better broken up into several individual tests instead.
| $this->assertSame( $expected, $output, $message ); | ||
| } | ||
|
|
||
| public function data_standalone_inline_script() { |
There was a problem hiding this comment.
Same comment about the use of a data provider here.
Fixes #17
Trac ticket:
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.