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

bootscore_ie_alert() gets cached #37

Closed
Axos11 opened this issue Nov 19, 2021 · 11 comments · Fixed by #53
Closed

bootscore_ie_alert() gets cached #37

Axos11 opened this issue Nov 19, 2021 · 11 comments · Fixed by #53
Labels
bug Something isn't working
Projects

Comments

@Axos11
Copy link
Contributor

Axos11 commented Nov 19, 2021

I just had the issue on a client site that the function bootscore_ie_alert() output gets actually pagecached if the first user to open the site after a cache clear is a IE User. Also, the function is not triggered if the page is already cached. As caching is very important for the sites speed i would suggest to move that check to an js function or an ajax call to prevent it beeing cached.

@Axos11 Axos11 added the bug Something isn't working label Nov 19, 2021
@crftwrk crftwrk added this to To do in 5.2.0.0 via automation Nov 19, 2021
@crftwrk
Copy link
Member

crftwrk commented Nov 19, 2021

Do you mean something like this window.alert("IE detected");?

I'm more a friend of generally removing this function. Bootstrap were the first who dropped the support for IE but many others like WooCommerce followed quickly. Created this function when doing the step from v4 to v5 to make clear that there is no support for this browser anymore. But now we are at the end of 2021 and there are polyfills to make Bootstrap 5 compatible with IE 11.

But many users likes this function. So, not sure to keep it or remove it. Mentioned this point already in v6 project. A js version will give us more control to remove it later again instead using a hook and function. On the other hand I try to remove js wherever it's possible. So, no idea what's the best at this point.

@Axos11
Copy link
Contributor Author

Axos11 commented Nov 19, 2021

I think the function kind of "helps" pushing people to modern browsers. And I agree that eliminating js where possible is a good idea (maybe could add it as a plugin?)

We could insert it the same way it is right now or as an Overlay. However, it has to be inserted into the page with js in my opinion because I have no idea how to avoid the page cache otherwise.

@justinkruit
Copy link
Member

I think the js isn't even that heavy, just an user agent check and go from there. You can even keep the alert in html and only remove d-none class when the user agent matches.

@Axos11
Copy link
Contributor Author

Axos11 commented Nov 23, 2021

I would just try to put the alert as far as possible to the bottom in the html to prevent Google from catching it somehow as a important component when it's crawling. Could work as a toast then or we could just move the node by js.

I would prefer the toast but what are your thoughts about this? What would you prefer?

@crftwrk
Copy link
Member

crftwrk commented Nov 24, 2021

In an earlier version was a pure HTML and CSS solution using <!--[if IE]>...<![endif]--> and @media all and (-ms-high-contrast: none), (-ms-high-contrast: active) {...}. But I was afraid that Google could crawl that. So, decided to use a PHP function instead.

Toasts are very cool, but is IE able to show them? https://www.browserling.com/browse/win/7/ie/11/https%3A%2F%2Fgetbootstrap.com%2Fdocs%2F5.1%2Fcomponents%2Ftoasts%2F

Have no real IE to test here.

@Axos11
Copy link
Contributor Author

Axos11 commented Nov 25, 2021

It sadly doesn't completely. It seems that with bootstrap 5.0.3 it worked but 5.1 kinda broke IE support completely. So the pages start to look really ugly if opened with IE.

Because of that i would prefer a fullscreen overlay (like in this example: https://www.pinterest.com/pin/664069907535455886/) to not show off a broken page.

@crftwrk
Copy link
Member

crftwrk commented Nov 25, 2021

Agree

@crftwrk crftwrk linked a pull request Dec 2, 2021 that will close this issue
@crftwrk
Copy link
Member

crftwrk commented Dec 2, 2021

As mentioned in PR #52, we cannot roughly delete IE alert function without crashing users child-themes. Because if we remove function and users have still <?php bootscore_ie_alert(); ?> hook in their header.php they will see the "white site of death".

Would suggest this steps:

Bootstrap 5.2 comes closer https://github.com/twbs/bootstrap/projects. Think they release in 1-2 weeks. By this release users get informed that they have to change something in their theme anyway.

5.2.0.0

  • We delete <?php bootscore_ie_alert(); ?> now, inform in blog post and newsletter that users should remove the hook in header.php. So, temporarily no IE Alert.

5.2.1.0

  • Because users have removed this we can delete now IE function in template-tags.php safely
  • Adding the js version.

In PR is this:

'<h1>' + /*esc_html__('Internet Explorer detected', 'bootscore')*/'Internet Explorer detected' + '</h1>' +

The esc_html__('Internet Explorer detected', 'bootscore') is the php translation string. Does that work this way?

Do you agree?

@Axos11
Copy link
Contributor Author

Axos11 commented Dec 3, 2021

Wouldn't it be a possibility to just remove the code in "bootscore_ie_alert()" and make it an empty function? Then nothing actually breaks and we can inform everyone, that that line is deprecated and should be removed in Child themes. in Version 5.3 we remove the function completely.

Because it is actually non breaking we could add the .js asap.

I left the translation in the code but commented it out. I thought you might have a working wordpress .js translation solution already :)

@crftwrk
Copy link
Member

crftwrk commented Dec 3, 2021

Yes, that sounds good.

  • remove <?php bootscore_ie_alert(); ?> in header.php and inform user
  • empty the function in template-tags.php
  • add js alert
  • remove empty function later

Is it an option for you to place js in function with <script> tag? In this case we can do what we want.

I left the translation in the code but commented it out. I thought you might have a working wordpress .js translation solution already :)

I have no idea how to translate js. The alert is always in english i guess, but I don't care.

@justinkruit
Copy link
Member

justinkruit commented Dec 3, 2021

Translating things in the js files is done with wp_localize_script. You load the strings with that, and then recall them in tje js file.

@crftwrk crftwrk linked a pull request Dec 4, 2021 that will close this issue
@crftwrk crftwrk removed a link to a pull request Dec 4, 2021
@crftwrk crftwrk removed this from To do in 5.2.0.0 Dec 6, 2021
@crftwrk crftwrk added this to To do in 5.1.3.1 via automation Dec 6, 2021
@crftwrk crftwrk moved this from To do to In progress in 5.1.3.1 Dec 6, 2021
@crftwrk crftwrk removed this from In progress in 5.1.3.1 Jan 21, 2022
@crftwrk crftwrk added this to To do in 5.2.0.0 via automation Jan 21, 2022
@crftwrk crftwrk moved this from To do to In progress in 5.2.0.0 Jan 21, 2022
5.2.0.0 automation moved this from In progress to Done Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants