-
-
Notifications
You must be signed in to change notification settings - Fork 78.5k
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
Use Babel and ES6 in docs JS files #31607
Conversation
Thanks for the PR! I'm going to have a closer look in the next days, but
from a quick look I notice you don't use imports. Also please don't change
any listing configuration files.
…On Sun, Sep 6, 2020, 08:03 Tiger Oakes ***@***.***> wrote:
@NotWoods <https://github.com/NotWoods> requested review from
@twbs/js-review on: #31607 <#31607>
Use Babel and ES6 in documentation as a code owner.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#31607 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNJNWXXD73FWMDE3AOLSEMJZRANCNFSM4Q33762A>
.
|
I didn't realize adding imports was one of the goals, I'll take another look and see how Hugo's bundler works. I didn't mean to change the root eslint file but the subfile is nessecary. |
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.
babel should only run for our files and not all files
site/assets/js/application.js
Outdated
}) | ||
} | ||
const exampleModal = document.getElementById('exampleModal') | ||
exampleModal?.addEventListener('show.bs.modal', event => { |
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.
Can you revert the optional chaining changes and revert the .eslintrc.json change? We don't use optional chaining anywhere else right now and I'd like to be consistent.
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.
If I remove the eslint file, the linter will complain about any ES6 syntax (i.e. const
). It's set up to only affect files in the docs folder.
I'll revert the optional chains.
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.
NVM about the comment regarding the ESLint changes. Your changes are good since we have an upper eslintrc.json in site folder because the examples still use ES5.
@regisphilibert: how does this implementation look good to you? Is there any other way to achieve the same result without the temp file? Also, thanks @NotWoods for sticking with us, every thing to improve things is welcome :) |
@Johann-S what do you think about this? My concerns are:
I wonder if we could use esbuild later which should be a lot faster. |
It seems ok to me, but yes we can import our source code that's possible too, but it'll produce a bigger js file and we won't use the Bootstrap CDN |
We don't use a CDN for our docs :) |
Still worth finishing off @XhmikosR? |
I'll need to rebase the branch since it has conflicts. That being said, it's the wise thing to make things consistent with code. The only downside I see is that this slows down the build, hence why I've left the PR hanging. But I'll take another stab at it in the next days. |
ced555c
to
5168a3d
Compare
I think we can use https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach#browser_compatibility |
/CC @XhmikosR any opinion over this?
I think it's the last thing in order to merge |
@GeoSot Want to wait for that change, or merge this and make it in another PR? |
@mdo Better merge it . It is too minor to keep us down |
Fixes #30818
Updates the
assets/js
path inside ofsite
so that the code is passed through Babel, using the root Babel RC file.Additionally adds a ESLint config in the assets folder to enable a higher ecma version, but only for doc files.
I didn't pass
extra_js
scripts through Babel, since the equivalentextra_css
scripts in the Examples folder don't get processed. I can add this if desired.Preview: https://deploy-preview-31607--twbs-bootstrap.netlify.app/
TODO:
Array.from
instead ofArray.prototype.slice.call