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

Rewrite theme.js to remove jQuery #565

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

androidacy-user
Copy link
Contributor

jQuery quite frankly is a burdensome and weighty library in today's ES6 world. This file is one of the only files in bootscore that still depends on jQuery to function.

jQuery quite frankly is a burdensome and weighty library in today's ES6 world. This file is one of the only files in bootscore that still depends on jQuery to function.
@justinkruit
Copy link
Member

This file is one of the only files in bootscore that still depends on jQuery to function.

That's actually not completely true, there is also woocommerce/js/woocommerce.js and the insides of woocommerce/inc/ajax-add-to-cart.php. So these changes will not completely remove jQuery as dependency, at least not when using woocommerce.

@androidacy-user
Copy link
Contributor Author

This file is one of the only files in bootscore that still depends on jQuery to function.

That's actually not completely true, there is also woocommerce/js/woocommerce.js and the insides of woocommerce/inc/ajax-add-to-cart.php. So these changes will not completely remove jQuery as dependency, at least not when using woocommerce.

Ah yeah forgot as we don't use woocommerce. Nonetheless this commit allows us to remove jquery on most pages, and as it's a 45kb library that's a nice bonus.

@androidacy-user
Copy link
Contributor Author

I'm not entirely comfortable rewriting those files to remove jquery as we cannot test it. Still, this PR is a good starting point for people like us that may only have one or two plugins that still use jquery.

@crftwrk
Copy link
Member

crftwrk commented Sep 9, 2023

I had this on my list as well. It's good that already someone want to do this. Tested it and found 2 issues. Readded the jQuery again to show how this snippets should work:

  1. First snippet // Handle offcanvas links click event. does not work. Check https://bootscore.me/theme/scrollspy-onepager/, shrink browser to mobile view and click the links in menu. Offcanvas closes if one of the onepager-links is clicked. Snippet can be deleted if Offcanvas: Improve offcanvas drawer support for same-page navigation twbs/bootstrap#38514 is merged.
  2. Fourth snippet // Hide collapse-search when clicking outside of it. Does not work as well. Click on the same page the search-toggler in the navbar. Click 1px right next to the input-group search button - collapse stays open. Click somewhere else on the whitespace - collapse closes.

About the other 2 files @justinkruit mentioned. I'm a huge friend to have a standard and Vanilla JS there would be nice as well. But both files gets only enqueued if WooCommerce plugin is installed and entire WooCommerce' JS is jQuery. It will not make a huge different if we change them to Vanilla JS too. So, in this case I'm fine using Vanilla in theme.js and jQuery in WC scripts.

So users who haven't installed WooCommerce have a theme without jQuery dependency when this is merged.

Just for info:

I'm not entirely comfortable rewriting those files to remove jquery as we cannot test it

Simply setup a testing site, install WooCommerce and you can test them well. But as I wrote above, I think this is not important for now.

And while we are here, Internet Explorer warning seems to be outdated and many other browsers are not supported by Bootstrap 5 as well. Think we can delete this in a further update.

If we get those 2 issues fixed, I'm in. Let me know what you think.

@justinkruit
Copy link
Member

And when all is done and fixed, please request both me and @crftwrk as reviewer (if you're able to, not entirely sure if you can as PR author so please test it).

I'd like to fully test this when the bugs mentioned above are fixed.

@crftwrk crftwrk marked this pull request as draft September 9, 2023 18:19
@crftwrk
Copy link
Member

crftwrk commented Sep 9, 2023

Set this to draft to avoid accidentally merging.

@androidacy-user
Copy link
Contributor Author

I had this on my list as well. It's good that already someone want to do this. Tested it and found 2 issues. Readded the jQuery again to show how this snippets should work:

  1. First snippet // Handle offcanvas links click event. does not work. Check https://bootscore.me/theme/scrollspy-onepager/, shrink browser to mobile view and click the links in menu. Offcanvas closes if one of the onepager-links is clicked. Snippet can be deleted if Offcanvas: Improve offcanvas drawer support for same-page navigation twbs/bootstrap#38514 is merged.
  2. Fourth snippet // Hide collapse-search when clicking outside of it. Does not work as well. Click on the same page the search-toggler in the navbar. Click 1px right next to the input-group search button - collapse stays open. Click somewhere else on the whitespace - collapse closes.

About the other 2 files @justinkruit mentioned. I'm a huge friend to have a standard and Vanilla JS there would be nice as well. But both files gets only enqueued if WooCommerce plugin is installed and entire WooCommerce' JS is jQuery. It will not make a huge different if we change them to Vanilla JS too. So, in this case I'm fine using Vanilla in theme.js and jQuery in WC scripts.

So users who haven't installed WooCommerce have a theme without jQuery dependency when this is merged.

Just for info:

I'm not entirely comfortable rewriting those files to remove jquery as we cannot test it

Simply setup a testing site, install WooCommerce and you can test them well. But as I wrote above, I think this is not important for now.

And while we are here, Internet Explorer warning seems to be outdated and many other browsers are not supported by Bootstrap 5 as well. Think we can delete this in a further update.

If we get those 2 issues fixed, I'm in. Let me know what you think.

If WC depends largely on jquery (which, honestly, i suspected it did but couldn't confirm) there's not much benefit in rewriting it's associated JS in bootscore, as the overall idea here is more optimized code + removing the need for jQuery. It's honestly disappointing such a major plugin still depends on jQuery but so does WordPress so it only makes sense.

The other two points i'll check as I have time, unfortunately a bit busy today and tomorrow haha.

@androidacy-user androidacy-user marked this pull request as ready for review October 19, 2023 19:41
@androidacy-user
Copy link
Contributor Author

@crftwrk ready to merge when you are

@crftwrk
Copy link
Member

crftwrk commented Oct 20, 2023

Nearly ;-)

  • I miss the comments and have to read the code first to understand what each snippet does.
  • Self closing offcanvas by click on <a> does still not work
  • NOT tested IE warning, because my old machine has gone. But as mentioned above, think we should remove this snippet in a separate PR.
  • All other snippets works fine
  • All snippets are wrapped in function initializeScripts() {. Why?

Changed some code and everything (IE warning not tested but not important) works fine. Is already live on bootscore.me.

/*--------------------------------------------------------------
Theme JS
--------------------------------------------------------------*/

// Close offcanvas on click a, keep .dropdown-menu open 
// See https://github.com/bootscore/bootscore/discussions/347
document.addEventListener('DOMContentLoaded', function () { // Fire after bootstrap.bundle.min.js is loaded 
  const offcanvasLinks = document.querySelectorAll('.offcanvas a:not(.dropdown-toggle):not(.remove_from_cart_button)');
  const offcanvas = new bootstrap.Offcanvas(document.querySelector('.offcanvas'));

  offcanvasLinks.forEach(function (link) {
    link.addEventListener('click', function () {
      offcanvas.hide();
    });
  });
});


// Remove search collapse button hide if widget is empty
// Deprecated v5.2.3.4, done by php if (is_active_sidebar('top-nav-search')) in header.php
// Remove in v6
const collapseSearch = document.getElementById('collapse-search');
if (collapseSearch && collapseSearch.children.length === 0) {
  document.querySelectorAll('.top-nav-search-md, .top-nav-search-lg').forEach((element) => element.remove());
}


// Focus searchform on click search-toggler
document.querySelector('#collapse-search').addEventListener('shown.bs.collapse', function () {
  document.querySelector('.top-nav-search input:first-of-type').focus();
});


// Close search collapse if click outside the searchform
document.addEventListener('click', function (event) {
  const target = event.target;
  const collapseSearch = document.querySelector('#collapse-search');
  const collapseButton = document.querySelector('[data-bs-target="#collapse-search"]');

  if (!collapseSearch.contains(target) && collapseSearch.classList.contains('show')) {
    collapseButton.click(); // Simulate a click on the collapse button to hide the collapse
  }
});


// To-top button
const topButton = document.querySelector('.top-button');
if (topButton) {
  window.addEventListener('scroll', function () {
    topButton.classList.toggle('visible', window.scrollY >= 500);
  });
}


// Height classes, add class to your content
['height-50', 'height-75', 'height-85', 'height-100'].forEach((cls) => {
  const factor = parseFloat(cls.split('-')[1]) / 100;
  document.querySelectorAll(`.${cls}`).forEach((element) => {
    element.style.height = `${window.innerHeight * factor}px`;
  });
});


// IE warning
if (window.document.documentMode && typeof bootscore !== 'undefined') {
  const IEWarningDiv = document.createElement('div');
  IEWarningDiv.className = 'position-fixed top-0 end-0 bottom-0 start-0 d-flex justify-content-center align-items-center';
  IEWarningDiv.style.backgroundColor = 'white';
  IEWarningDiv.style.zIndex = 1999;
  IEWarningDiv.innerHTML = `
        <div style="max-width: 90vw;">
            <h1>${bootscore.ie_title}</h1>
            <p class="lead">${bootscore.ie_limited_functionality}</p>
            <p class="lead">${bootscore.ie_modern_browsers_1}${bootscore.ie_modern_browsers_2}${bootscore.ie_modern_browsers_3}${bootscore.ie_modern_browsers_4}${bootscore.ie_modern_browsers_5}</p>
        </div>`;
  document.body.appendChild(IEWarningDiv);
}

How to test

  • Go to https://bootscore.me/theme/scrollspy-onepager/. Each section should have 100% window height.
  • Shrink browser on the same page to mobile view, open offcanvas and click a nav-link. Offcanvas closes.
  • Click the search-toggler in the navbar. Searchform gets focused.
  • Click 1px left or right next to the searchform. Searchform loses focus but collapse stays open.
  • Click somewhere else on the page. Searchform collapse closes.
  • Add an outdated header.php to your child. Now go in backend to widgets and remove the searchform in top-nav-search area. Search toggler should be removed in frontend.

If you want to change this, I'm in 👍

@androidacy-user
Copy link
Contributor Author

androidacy-user commented Oct 22, 2023

Nearly ;-)

  • I miss the comments and have to read the code first to understand what each snippet does.
  • Self closing offcanvas by click on <a> does still not work
  • NOT tested IE warning, because my old machine has gone. But as mentioned above, think we should remove this snippet in a separate PR.
  • All other snippets works fine
  • All snippets are wrapped in function initializeScripts() {. Why?

Changed some code and everything (IE warning not tested but not important) works fine. Is already live on bootscore.me.
If you want to change this, I'm in 👍

1/ just overlooked tbh
2/ i honestly wasn't aware theme.js did that as we don't have any on-page links in our offcanvas, sorry
3/ i don't have ie to test 😉
4/ yay!
5/ to handle situations where users may be using a defer javascript plugin and the DOM may already be loaded when the script fires (as is the case on androidacy.com)

@crftwrk
Copy link
Member

crftwrk commented Oct 23, 2023

The thing is that each snippet has a reason why it's there and has a job to do. If one of this is not working, it may create issues on existing installations. I recommend to setup a test environment instead doing changes on your live site.

Agree that we can skip to test the IE warning, but all other snippets have to work properly. Comments are mandatory as well to help other contributors to understand what each snippet does.

Added in #565 (comment) a working script for the self-closing offcanvas.

Set this to draft again to avoid accidentally merging. If we can fix that, I'm in 👍

@crftwrk crftwrk marked this pull request as draft October 23, 2023 10:54
@crftwrk
Copy link
Member

crftwrk commented Nov 11, 2023

@androidacy-user FYI, IE warning has been removed #620. You can skip this snippet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants