-
Notifications
You must be signed in to change notification settings - Fork 132
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
Replace jQuery with vanilla JS #2130
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this @yucheng11122017
@ang-zeyu I suppose we will want to remove the inclusion of jQuery assets after all direct usage has been removed, is that right? I think Bootstrap 5 no longer needs jQuery.
If so, we will need to remove the static asset and update the documentation, and possibly the functional tests as well.
Should I do this in this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I do this in this PR?
I hadn't investigated thoroughly whether there are other dependencies relying on jQuery, but if you have the time, please do!
It'd be very impractical to retest everything for a dependency removal as well, and we don't have e2e tests. Some basic verification we can do is to open all the pages in the documentation and check for console errors after investigating the dependencies.
Float
I'm not aware of this as well, but if we've confirmed this is undocumented, we can skip testing this.
…moveJquery # Conflicts: # packages/vue-components/src/panels/PanelBase.js
Seems to be causing an issue with panel including segments from elsewhere. Following warning is logged in the browser console:
|
Thanks for noticing this issue @tlylt Upon some further investigation, the issue isnt the box but rather the code in the content that is triggering the warning. In Retriever.vue, a new html document is made to load in the html to query for the hash. Edit: SVG file doesn't contain any inline styling except for height and width but is still triggering the warning for some reason Removing the codeWrapButton plugin or just commenting off the SVG file in the codeWrapButton or removing the SVG file will cause the warning to go away Edit: Removing the height and width does not get rid of the warning This issue has been documented at this thread Vue stripping out styles from inline SVG where the suggested fix is to modify the SVG file by moving the CSS out This happens regardless of whether a fragment is used or not. Previously with jQuery, a seperate self-closed element was created hence it does not affect the document. Things I have tried so far:
|
So does it work?
What do you mean by "a separate self-closed element" here? Is it possible to fix the SVG code in codeWrapButton such that it does not cause the issue (without breaking the functionality of the plugin)? And is it possible to automatically adjust the SVG code when you query the fragment and realize that there's an SVG inside? |
Sorry for the late response @tlylt! Update on the issue I misinterpreted the issue because for some reason when removing the height and width of the SVG and using only one file, the warning disappeared. Its not about the style but about the fact that the live server was injecting a script to enable the SVG file. After removing the script tag, the warning no longer appears. Thank you to @EltonGohJH, @lhw-1 and @itsyme for discussing the issue with me and your suggestions! |
So the script that live-server injected got removed, but it is ok because this injected script is in the temporary HTML document created solely for retrieving the hash? |
Yup. All scripts should be ok to remove because even if its inside, Vue does not process it because of the side effects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you @yucheng11122017, could you help write the breaking change guide in the PR description? To be safe we should declare this a breaking change, as it was stated that jQuery is included in our assets. Therefore, users of MarkBind might inadvertently use jQuery in their custom JavaScript code (if any). Just a short explanation will do, stating that if they used jQuery in their pages or any of their custom JS code, they should either import jQuery or convert the usage to native js.
Updated the PR description under breaking change description! |
@yucheng11122017 could you help with fixing the conflict? |
# Conflicts: # packages/core/src/Site/index.ts
…moveJquery # Conflicts: # packages/core/src/Site/index.ts
Done! |
Something we should try to fix still as usage of Filed #2300 for it |
What is the purpose of this pull request?
Overview of changes:
Change instances of jQuery to use vanilla JS as requested in #1926
Closes #1926
Anything you'd like to highlight/discuss:
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Checklist: ☑️
Breaking change description:
jQuery is no longer supported in Markbind. This is because jQuery is no longer as relevant since it can be replaced with modern DOM APIs.
If jQuery is used in your website, either