the-events-calendar / the-events-calendar-category-colors

Plugin add-on for The Events Calendar to colorize categories
https://wordpress.org/plugins/the-events-calendar-category-colors
44 stars 19 forks source link

Testing legend-superpowers #150

Closed afragen closed 1 year ago

afragen commented 1 year ago

@barryhughes on my test site, https://tribe.thefragens.net, I am having issues on mobile. Doesn't seem to be able to change view or select legend menu. 🧐

Almost as if JS not loading on mobile.

barryhughes commented 1 year ago

Something is definitely afoot, though I'm not sure what. On your site (check the exposed and weirdly spaced span tags):

span-tags-funky-town

Yet locally I don't see the same when I drop into mobile view:

legend-ok

Will need to play and experiment a little 🕵🏼

afragen commented 1 year ago

I'm not seeing that either on laptop or iPhone.

barryhughes commented 1 year ago

OK, that's odd ... now I consistently see the < span > ... < / span > on all devices (when looking at tribe.thefragens.net, I mean) but I was sure when I last looked it was only on mobile.

barryhughes commented 1 year ago

Wait, I think I see what's happening (and the reason I didn't see this locally is simply because I was still testing with my branch). Here's the recent commit history for the develop branch:

Looking at the second-from-last commit ("wpcs"), we can see formatting fixes, but also changes like this one:

- const replacementText = `<span>${linkTitle}</span>`;
+ const replacementText = ` < span > ${linkTitle} < / span > `;

That might explain some of the weirdness.

afragen commented 1 year ago

I see it too now. Makes me wonder why WPCS for JS would do that. Let me fix it.

afragen commented 1 year ago

These are the "errors" phpcs is detecting.

137 | ERROR   | [x] Expected 1 space before "<"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 137 | ERROR   | [x] Expected 1 space after "<"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
 137 | ERROR   | [x] Expected 1 space before ">"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 137 | ERROR   | [x] Expected 1 space after ">"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
 137 | ERROR   | [x] Expected 1 space before "<"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 137 | ERROR   | [x] Expected 1 space after "<"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
 137 | ERROR   | [x] Expected 1 space before "/"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 137 | ERROR   | [x] Expected 1 space after "/"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
 137 | ERROR   | [x] Expected 1 space before ">"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 137 | ERROR   | [x] Expected 1 space after ">"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
afragen commented 1 year ago

My guess is it has to do with being inside of backticks.

barryhughes commented 1 year ago

Yep, those rules probably make sense when applied outside of a string, in a comparison or when doing arithmetic. It would seem like a bug for them to be applied inside a string and outside of interpolated fragments.

That is, it might make sense to change:

const myString = `<p>Hello ${ age<15 ? 'kid' : 'shipmate' }</p>`

Into:

const myString = `<p>Hello ${ age < 15 ? 'kid' : 'shipmate' }</p>`

But not:

const myString = ` < p > Hello ${ age < 15 ? 'kid' : 'shipmate' } < /p > `
barryhughes commented 1 year ago

Interesting: https://github.com/squizlabs/PHP_CodeSniffer/issues/2594.

Perhaps it is better to switch away to eslint or similar (for JS)?

afragen commented 1 year ago

I updated phpcs.xml to not parse *.js

barryhughes commented 1 year ago

Let me know if you continue to see any odd behavior: I poked around and things are looking stable again when I test locally with the latest develop branch code.

(Though I do still see some weirdness on your test site, but I believe that is unrelated to these changes ... for instance, trying to switch to month view doesn't work, and I see the HTML response is 403 Forbidden.)

afragen commented 1 year ago

Seems to be working in latest testing. I'll see about merging and release next week.