Fix external styles and script not hoisted #2414

yiwen101 · 2024-02-12T07:11:01Z

What is the purpose of this pull request?

Resolve issue: #2300

Overview of changes:
Widen the scope of styles and script hoisting to include external and external manager.

Anything you'd like to highlight/discuss:

Testing instructions:
Init a markbind sample project.
In contents/topic1.md, add:

<h1>
<script>
      // JavaScript code specific to this component
      console.log("Inline script executed! 35");
</script>
<style>
      /* CSS styles specific to this component */
      h1 {
        color: red;
      }
</style>
h1 text
</h1>

In index.md, add

<panel src = "contents/topic1.md">
</panel>

You should see the following.The styles in the contents/topic1.md is applied to the content in the panel successfully.

Proposed commit message: (wrap lines at 72 characters)

Hoist external's styles and script
Also add warning in documentation that styles in hoisted
external will also affect the rest of the page

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update Documentation on Markdown in Presentation Components

codecov · 2024-02-12T07:13:40Z

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 48.92%. Comparing base (db27d8a) to head (ecfab66).

Files	Patch %	Lines
packages/core/src/External/External.ts	0.00%	3 Missing ⚠️
packages/core/src/External/ExternalManager.ts	0.00%	2 Missing ⚠️
packages/core/src/Page/index.ts	0.00%	1 Missing ⚠️
...ckages/core/src/html/scriptAndStyleTagProcessor.ts	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2414   +/-   ##
=======================================
  Coverage   48.92%   48.92%           
=======================================
  Files         124      124           
  Lines        5245     5245           
  Branches     1110     1110           
=======================================
  Hits         2566     2566           
+ Misses       2372     2371    -1     
- Partials      307      308    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kaixin-hc

Just a preliminary look - do update this PR so that it can pass the checks!

docs/userGuide/components/presentation.md

packages/core/src/External/External.ts

yucheng11122017 · 2024-02-15T09:28:36Z

Hi @yiwen101 could you also add functional test cases? You can add it to test_site :)

yiwen101 · 2024-02-15T09:56:49Z

Hi @yiwen101 could you also add functional test cases? You can add it to test_site :)

Definitely. Thank you for pointing out to this.
I did not look at the test in the cli package and not aware of the exist of functional test. It looks like a wonderful way to test the code. Will work on that.

This reverts commit ff5a4a6.

EltonGohJH · 2024-02-16T06:22:31Z

@kaixin-hc @yucheng11122017 @itsyme

I think all in all looks okay to me from my testing. It seems that the style and script is hosited for Externals correctly. Feel free to chime in as I may have missed something!

For the code cov check failure, we can ignore it right? (as externals is not tested in the first place)
I think one point I am concerned about is that the style get hoisted so it becomes global to the page. So, lets say you make H1 red, it will become global to the page. Do you think we needed to add this the documentation? Since, I think it might be unexpected behaviour for most people. (Would there be unexpected consequence of a global hositing?)

yiwen101 · 2024-02-16T06:47:36Z

@kaixin-hc @yucheng11122017 @itsyme

I think all in all looks okay to me from my testing. It seems that the style and script is hosited for Externals correctly. Feel free to chime in as I may have missed something!

For the code cov check failure, we can ignore it right? (as externals is not tested in the first place)

I think one point I am concerned about is that the style get hoisted so it becomes global to the page. So, lets say you make H1 red, it will become global to the page. Do you think we needed to add this the documentation? Since, I think it might be unexpected behaviour for most people. (Would there be unexpected consequence of a global hositing?)

Thanks for the comment; yes this is a very valid concern.
Will modify the documentation to warn this unintended effect of including source that contains script and css.

How do you find the idea of adding a new attribute to allow the user to choose whether to hoist the css and script? Cause I feel a documentation warning is best when complemented with feature that providing user with more options. If you approve the idea, I will open a separate issue and work on adding the attribute in addition to documentation fix.

Meanwhile, s commented in #2300, probably worth opening a new issue on "finding a more proper fix that retains the script element placement instead of hoisting it downward." as well?

EltonGohJH · 2024-02-16T08:16:47Z

@yiwen101
I think that I am not sure how useful that attribute would be. It would completely disable the style and script for that external component. What do you think?

Meanwhile, s commented in #2300, probably worth opening a new issue on "finding a more proper fix that retains the script element placement instead of hoisting it downward." as well?

For CSS wise, it is global because the styling is not unique to the element.

For script wise, I am not super sure whether it matters whether element placment matters (I do see that maybe we want the script to trigger when the element is shown but I do not think that this is trivial to implement).

yiwen101 · 2024-02-16T08:35:14Z

@yiwen101 I think that I am not sure how useful that attribute would be. It would completely disable the style and script for that external component. What do you think?

Meanwhile, s commented in #2300, probably worth opening a new issue on "finding a more proper fix that retains the script element placement instead of hoisting it downward." as well?

For CSS wise, it is global because the styling is not unique to the element.

For script wise, I am not super sure whether it matters whether element placment matters (I do see that maybe we want the script to trigger when the element is shown but I do not think that this is trivial to implement).

I see;

Regarding the usefulness of the attribute, if I am correct in my understanding, without this attribute, I will need to mention in documentation that:
1 Including a source with style will cause that to be applied globally
2 If do not want this, go to copy paste from the source rather than use "src= ...". ?

Point 2 does not look graceful. By adding a new attribute (not sure how long will take me though), we can replace point 2 with "If do not want this, add importStyle = false ... ", so it looks more elegant?

EltonGohJH · 2024-02-21T02:58:19Z

@yiwen101
SFLR.

I think that would be okay. You can add the documentations to Panels and Includes. I cant remember other than that where do we use this so do double check.
I think you can give it a try if you really want. I feel that there is no need to go to such an extent as this is a feature that is not commonly used. It can be circumvented by copy pasting as stated by you.

kaixin-hc · 2024-02-22T07:08:18Z

1 Including a source with style will cause that to be applied globally
2 If do not want this, go to copy paste from the source rather than use "src= ...". ?

@yiwen101 yep, just make this documentation change (you can also suggest that in the styles of the includes they could specifically target a class name that's not used globlly) and i think this PR is ready to go!

yiwen101 · 2024-02-23T05:17:24Z

@yiwen101 SFLR.

I think that would be okay. You can add the documentations to Panels and Includes. I cant remember other than that where do we use this so do double check.

I think you can give it a try if you really want. I feel that there is no need to go to such an extent as this is a feature that is not commonly used. It can be circumvented by copy pasting as stated by you.

1 Including a source with style will cause that to be applied globally
2 If do not want this, go to copy paste from the source rather than use "src= ...". ?

@yiwen101 yep, just make this documentation change (you can also suggest that in the styles of the includes they could specifically target a class name that's not used globlly) and i think this PR is ready to go!

I have updated the documentation required. Thank you for all your helps and comments throughout the process.

kaixin-hc

Minor nits on the documentation - but let me leave my approval here first : )

docs/userGuide/syntax/panels.md

yucheng11122017

LGTM! Thank you for all the changes.
@EltonGohJH you ok with this PR? If yes I think it can be merged!
Should it be considered a breaking change?

EltonGohJH · 2024-02-25T06:07:51Z

@yucheng11122017
I agree that it can be merged.
I do not think it is a breaking change because it is more of a bug fix.
Users would expect that their external script and css to work but it does not work before this PR.
What do you think?

yucheng11122017 · 2024-02-25T06:40:37Z

@yucheng11122017 I agree that it can be merged. I do not think it is a breaking change because it is more of a bug fix. Users would expect that their external script and css to work but it does not work before this PR. What do you think?

OK sure sounds good! I was more concerned abt the global effect of script and css that was discussed here that made it sound like a breaking change but yeah I think bug fix sounds more appropriate.

kaixin-hc · 2024-02-25T11:49:01Z

@all-contributors add @yiwen101 for code

allcontributors · 2024-02-25T11:49:12Z

@kaixin-hc

I've put up a pull request to add @yiwen101! 🎉

yiwen101 and others added 25 commits February 3, 2024 16:58

Update trouble shooting

3ae1a6f

Update code examples in affected presentation components

9da62fa

Update message for presentation component

24fa82e

Revert unrelated changes

a99d972

Fix links

9c4b9a5

Update docs/userGuide/components/presentation.md

3c16fb3

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Merge branch 'master' into update_doc

2009e41

Revert distracting changes made by formatter

fdfdc8b

Resolve merge conflict

b9e4197

Revert distracting changes by formatter2

420ceba

Migrate to codeAndOutput

f2b511b

Updade doc

eb6f783

Update presentation.md

0cd7a61

Update ug

c81bb3d

Clean formatting error

a282b01

Proof read

54e1d86

Improve formatting

1d90406

Improve format

2fc7094

Improve format

15b6532

Update troubleshooting.md

f7ca43f

Update troubleshooting.md

ac5e734

Merge pull request MarkBind#2399 from yiwen101/update_doc

220cde2

Update Documentation on Markdown in Presentation Components

Preliminary solution to issue

710a3cb

Clean debug logs

14aa7a2

Lint the code

194b5bf

yucheng11122017 force-pushed the master branch from 220cde2 to a265c73 Compare February 12, 2024 13:20

kaixin-hc reviewed Feb 13, 2024

View reviewed changes

docs/userGuide/components/presentation.md Outdated Show resolved Hide resolved

packages/core/src/External/External.ts Show resolved Hide resolved

Merge branch 'master' into hoist_external

eabd88f

yiwen101 marked this pull request as draft February 13, 2024 09:37

yiwen101 added 2 commits February 15, 2024 15:35

Merge branch 'MarkBind:master' into hoist_external

eafb926

Merge branch 'master' into hoist_external

bd62828

yiwen101 added 4 commits February 15, 2024 19:53

Add functional test

0dbc4fe

Update testSourceContainScript.html

4491d0c

Help the PR to pass the code coverage test.

ff5a4a6

Revert "Help the PR to pass the code coverage test."

49cdac6

This reverts commit ff5a4a6.

yiwen101 marked this pull request as ready for review February 15, 2024 15:05

yiwen101 and others added 2 commits February 22, 2024 17:09

Merge branch 'master' into hoist_external

75a2eae

Add warning in user doc

5c7bc46

kaixin-hc approved these changes Feb 23, 2024

View reviewed changes

docs/userGuide/syntax/panels.md Outdated Show resolved Hide resolved

docs/userGuide/syntax/panels.md Outdated Show resolved Hide resolved

yiwen101 and others added 3 commits February 23, 2024 14:02

Improve docs

40b33ac

Merge branch 'master' into hoist_external

1687a13

Update snapshot after merging master

a847d1a

yucheng11122017 approved these changes Feb 25, 2024

View reviewed changes

Merge branch 'master' into hoist_external

ecfab66

kaixin-hc merged commit ef502db into MarkBind:master Feb 25, 2024
9 of 10 checks passed

allcontributors bot mentioned this pull request Feb 25, 2024

add yiwen101 as a contributor for code #2437

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix external styles and script not hoisted #2414

Fix external styles and script not hoisted #2414

yiwen101 commented Feb 12, 2024 •

edited by kaixin-hc

codecov bot commented Feb 12, 2024 •

edited

kaixin-hc left a comment

yucheng11122017 commented Feb 15, 2024

yiwen101 commented Feb 15, 2024

EltonGohJH commented Feb 16, 2024

yiwen101 commented Feb 16, 2024

EltonGohJH commented Feb 16, 2024

yiwen101 commented Feb 16, 2024

EltonGohJH commented Feb 21, 2024

kaixin-hc commented Feb 22, 2024

yiwen101 commented Feb 23, 2024

kaixin-hc left a comment

yucheng11122017 left a comment

EltonGohJH commented Feb 25, 2024

yucheng11122017 commented Feb 25, 2024

kaixin-hc commented Feb 25, 2024

allcontributors bot commented Feb 25, 2024

Fix external styles and script not hoisted #2414

Fix external styles and script not hoisted #2414

Conversation

yiwen101 commented Feb 12, 2024 • edited by kaixin-hc

Hoist external's styles and script Also add warning in documentation that styles in hoisted external will also affect the rest of the page

codecov bot commented Feb 12, 2024 • edited

Codecov Report

kaixin-hc left a comment

Choose a reason for hiding this comment

yucheng11122017 commented Feb 15, 2024

yiwen101 commented Feb 15, 2024

EltonGohJH commented Feb 16, 2024

yiwen101 commented Feb 16, 2024

EltonGohJH commented Feb 16, 2024

yiwen101 commented Feb 16, 2024

EltonGohJH commented Feb 21, 2024

kaixin-hc commented Feb 22, 2024

yiwen101 commented Feb 23, 2024

kaixin-hc left a comment

Choose a reason for hiding this comment

yucheng11122017 left a comment

Choose a reason for hiding this comment

EltonGohJH commented Feb 25, 2024

yucheng11122017 commented Feb 25, 2024

kaixin-hc commented Feb 25, 2024

allcontributors bot commented Feb 25, 2024

yiwen101 commented Feb 12, 2024 •

edited by kaixin-hc

Hoist external's styles and script
Also add warning in documentation that styles in hoisted
external will also affect the rest of the page

codecov bot commented Feb 12, 2024 •

edited