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

Add config to disable Requirements::customScript #200

Open
wants to merge 5 commits into
base: 4
Choose a base branch
from

Conversation

KINKCreative
Copy link

@KINKCreative KINKCreative commented Jan 15, 2020

Adding a configuration to disable injecting the custom JavaScript code into the pages. Also added a custom ID to the Requirements::customScript, as per best practices, to allow blocking the requirement programatically.

My use case: I am including the silverstripe/elemental-flexslider but am using completely my own templates and all the frontend code. I have no way to exclude the custom JS required with this module. I also think a configuration is a simpler step than clearing the requirement.

@codecov
Copy link

codecov bot commented Jan 15, 2020

Codecov Report

Merging #200 into 4 will decrease coverage by 0.57%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##                  4     #200      +/-   ##
============================================
- Coverage     54.64%   54.06%   -0.58%     
- Complexity       80       81       +1     
============================================
  Files             5        5              
  Lines           280      283       +3     
============================================
  Hits            153      153              
- Misses          127      130       +3
Flag Coverage Δ Complexity Δ
#php 54.06% <0%> (-0.58%) 81 <0> (+1)
Impacted Files Coverage Δ Complexity Δ
src/ORM/FlexSlider.php 68.75% <0%> (-1.9%) 21 <0> (+1)

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 cffdbcd...ba5a67e. Read the comment docs.

@muskie9 muskie9 changed the base branch from 4.1 to 4 January 15, 2020 00:29
@muskie9 muskie9 self-requested a review January 15, 2020 00:29
Copy link
Member

@muskie9 muskie9 left a comment

Choose a reason for hiding this comment

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

Overall it works as expected, thanks! Just a couple updates noted.

docs/en/index.md Outdated Show resolved Hide resolved
src/ORM/FlexSlider.php Outdated Show resolved Hide resolved
@KINKCreative
Copy link
Author

Would it be helpful to list the arbitrary ID of the custom script include in the docs, in case someone wants to block / update (? if possible) the requirement in their code.

@muskie9
Copy link
Member

muskie9 commented Jan 15, 2020

I think that's a great idea. Now that we have it, and there are clear use cases, it's worth noting for easy reference.

@@ -205,6 +205,11 @@ public function contentcontrollerInit()
*/
public function getCustomScript()
{

if ($this->owner->config('clear_requirements')) {
Copy link
Member

@muskie9 muskie9 Jan 15, 2020

Choose a reason for hiding this comment

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

Sorry, one other thing I just noticed. I believe this implementation will always return true, as it will return the full config for $this->owner. Updating to $this->owner->config()->get('clear_requirements') would have the expected result.

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.

2 participants