Added if statement and replaced jQuery with Vanilla JS#76
Added if statement and replaced jQuery with Vanilla JS#76PlanB007 wants to merge 2 commits intoWordPress:masterfrom
Conversation
… have this snippet on all the php files. It's up to you if you want that. Pro tip: it's safer
| 'clip': 'rect(1px, 1px, 1px, 1px)', | ||
| 'position': 'absolute' | ||
| } ); | ||
| document.querySelector( '.site-title, .site-description' ).style.clip = 'auto'; |
There was a problem hiding this comment.
Where we're changing multiple styles at the same time it may be preferrable to use the Object.assign syntax. For example:
Object.assign( document.querySelector( '.site-title, .site-description' ).style,
{
'clip': 'rect(1px, 1px, 1px, 1px)',
'position': 'absolute''
} );
There was a problem hiding this comment.
If you mean there are multiple .site-title and .site-description you can use Object.assign. I haven't used it before, always could solve it easy with just document.querySelector.
There was a problem hiding this comment.
There are multiple statements that start document.querySelector( '.site-title, .site-description' ).style to change more than one property of the element's style. Using Object.assign like this would mean that document.querySelector is only called once.
| $( '.site-title a, .site-description' ).css( { | ||
| 'color': to | ||
| } ); | ||
| document.querySelector( '.site-title, .site-description' ).style.clip = 'auto'; |
There was a problem hiding this comment.
Again Object.assign syntax could simplify this. There's also a whitespace issue here, it looks like spaces and not tabs at the start of the line.
There was a problem hiding this comment.
There maybe went something wrong with the indent/whitespace there, although it seems weird since it doesn't happen al the time.
There was a problem hiding this comment.
Its definitely spaces used instead of tabs
| @@ -1,4 +1,4 @@ | |||
| <?php | |||
| <?php if( ! defined( 'ABSPATH' ) ) exit; // Exit if accessed directly | |||
There was a problem hiding this comment.
It's often a good idea to use this check in themes and plugin files that are running code that access globals like $_GET and $_POST. That isn't the case here and so the check is probably unnecessary. If we were to add it then it should be a separate change in a new PR.
There was a problem hiding this comment.
Agree this is unnecessary here but doesn't hurt either. What does hurt is my OCD because it's before the file comment :p
There was a problem hiding this comment.
it should be a separate change in a new PR.
💯
| } )( jQuery ); | ||
| }); | ||
| }); | ||
| })( jQuery ); |
There was a problem hiding this comment.
Now that we've removed the dependency in jQuery we can remove it as a parameter. Alternatively, we can probably remove the closure altogether, which would simplify things further.
There was a problem hiding this comment.
This was marked resolved... looks like "jQuery" is still here? Also the $ parameter up top.
| 'color': to | ||
| } ); | ||
| document.querySelector( '.site-title, .site-description' ).style.clip = 'auto'; | ||
| document.querySelector( '.site-title, .site-description' ).style.position = 'relative'; |
| $( '.site-title a' ).text( to ); | ||
| } ); | ||
| } ); | ||
| document.querySelector( '.site-title a' ).textContent( to ); |
There was a problem hiding this comment.
I think this is invalid since textContent is not a function.
| document.querySelector( '.site-title a' ).textContent( to ); | |
| document.querySelector( '.site-title a' ).textContent = to; |
| $( '.site-description' ).text( to ); | ||
| } ); | ||
| } ); | ||
| document.querySelector( '.site-description' ).textContent( to ); |
There was a problem hiding this comment.
| document.querySelector( '.site-description' ).textContent( to ); | |
| document.querySelector( '.site-description' ).textContent = to; |
6 participants
As you can see by my commits I've added an if statement on one of your files. For WP this is a best practices pieces of code, you can find all the extra info on Google. For the jQuery, you used there was some very easy Vanilla JS which is, in this case, better than jQuery (check out youmightnotneedjquery.com ) always keep libs and frameworks as low as possible.