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.

Sign up for GitHub

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

Jump to bottom

Fix script tag helper dependencies inheriting the script's HTML attributes #15438

Merged
merged 18 commits into from
Mar 6, 2024
Merged

Fix script tag helper dependencies inheriting the script's HTML attributes #15438

merged 18 commits into from
Mar 6, 2024

Conversation

sarahelsaig
Copy link
Contributor

Also some code cleanup in ScriptTagHelper to keep it DRY.

Fixes #15375

sarahelsaig added 18 commits March 2, 2024 23:17
@sarahelsaig
Clean up if condition chain.
ffc1a8e
@sarahelsaig
Use await using where applicable.
e5fa042
@sarahelsaig
Remove duplicate definition.SetVersion call within the same if branch.
043b9a0
@sarahelsaig
DRY identical resource definition setter code from different branches.
5886a5b
@sarahelsaig
DRY identical RequireSettings setter code from different branches.
3abc99f
@sarahelsaig
DRY identical RequireSettings output writing code from different bran…
1038b03 
…ches.
@sarahelsaig
Restore accidentally deleted lines.
b088416
@sarahelsaig
output.Attributes is always iterated right after PopulateRequireSetti…
fda9698 
…ngs so let's bring it in to DRY.
@sarahelsaig
Fix random typo.
93b071e
@sarahelsaig
Minor code simplification.
409b8f1
@sarahelsaig
Split up RequireSettings.NewAndCombine.
4ca38b7
@sarahelsaig
Only cascade the location and position.
f71ec95
@sarahelsaig
Initialize RequireSettings using all applicable ResourceDefinition pr…
3465ec5 
…operties.
@sarahelsaig
Handle direct requires and expanded dependencies differently.
dc64579
@sarahelsaig
Merge branch 'main' into script-tag-helper-attributes
d55c671
@sarahelsaig
Merge branch 'main' into script-tag-helper-attributes
f9d2f84
@sarahelsaig
Fix it so test pass.
79be040
@sarahelsaig
Merge remote-tracking branch 'origin/script-tag-helper-attributes' in…
f5fa07c 
…to script-tag-helper-attributes
@Skrypt
Copy link
Contributor

Skrypt commented Mar 5, 2024 •
edited
Loading

I don't remember exactly why we were inheriting these script tag properties. I hope it won't break anything else. Maybe we should have unit tests to make sure that we cover the specs that were needed ... if we ever find them.

That's why unit tests are always helpful. At least, we then know which user cases it needs to cover. Now, we don't really know.

@sarahelsaig
Copy link
Contributor Author

To be fair this setting inheritance was set up 8 years ago in the same PR where the Orchard.ResourceManagement project was created. So it may just be a case of initial over-engineering.

@Skrypt
Copy link
Contributor

Skrypt commented Mar 5, 2024

Yeah and it was copied from Orchard 1 so add another 10 years on that. 😄

@sarahelsaig
Copy link
Contributor Author

Yeah, I've looked into it and guess what?

image

@Skrypt Skrypt merged commit 6f59635 into OrchardCMS:main Mar 6, 2024
4 checks passed
This was referenced Mar 13, 2024
Closed
Open
urbanit pushed a commit to urbanit/OrchardCore that referenced this pull request Mar 18, 2024
@sarahelsaig @urbanit
Fix script tag helper dependencies inheriting the script's HTML attri…
1bd3355 
…butes (OrchardCMS#15438)
This was referenced Apr 1, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Skrypt Skrypt Skrypt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Script type="module" issue
2 participants
@sarahelsaig @Skrypt