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

Regex may not always match the desired substring. #4

Closed
timothyjensen opened this issue Aug 22, 2017 · 10 comments
Closed

Regex may not always match the desired substring. #4

timothyjensen opened this issue Aug 22, 2017 · 10 comments

Comments

@timothyjensen
Copy link
Contributor

timothyjensen commented Aug 22, 2017

The regex used in the replace() method may not always match the substring 'no-js':

c = c.replace(/no-js/, 'js');

Say we have a body class of 'home alpha-plugin-no-js no-js', then the regex will match the first occurrence of 'no-js' and result in a body class of 'home alpha-plugin-js no-js'.

To avoid this, we can add the global flag to the regex, but that might conflict with other plugins. Another idea is to add a space so that the regex matches / no-js/ (with a preceding space). We could also create a more complex regex, but that might not be necessary.

@GaryJones
Copy link
Owner

The space won't match when the classname is the first class in the attribute value (i.e. has a " before it).

But you're right, this regex could be improved.

@GaryJones
Copy link
Owner

GaryJones commented Aug 23, 2017

@timothyjensen

How about a regex of ([" ])no-js([" ]), and a replace of $1js$2?

screenshot 2017-08-23 02 52 41

@timothyjensen
Copy link
Contributor Author

timothyjensen commented Aug 23, 2017

@GaryJones I tested that out on a demo site and it didn't seem to work for me, possibly because quotation marks don't occur in the search string.

How about (^|\s)(no-js)(?![^\s]) and replace with js (with a leading space)?

@timothyjensen
Copy link
Contributor Author

screen shot 2017-08-23 at 7 14 48 am

@GaryJones
Copy link
Owner

Better yet, why don't we use document.body.classList to grab the classes as an array instead?

var c = document.body.classList;
c = c.remove('no-js');
c = c.add('js');
document.body.classList = c;

@timothyjensen
Copy link
Contributor Author

Brilliant! That's much better than a complex regex.

@timothyjensen
Copy link
Contributor Author

Could reduce a few bytes with:

var c = document.body.classList;
c.remove('no-js');
c.add('js');

GaryJones added a commit that referenced this issue Aug 23, 2017
Instead of relying on a regex to amend a string, deal with the list of classes as an array.

Fixes #4.
@GaryJones
Copy link
Owner

@timothyjensen Feel free to checkout and give 70c2126 a go (or the rest of the develop branch, if you're running PHP 7.1+).

@timothyjensen
Copy link
Contributor Author

Great, I just tested develop on PHP 7.1.4 with no issues.

@GaryJones
Copy link
Owner

GaryJones commented Aug 24, 2017

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

No branches or pull requests

2 participants