WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#38752 closed defect (bug) (fixed)

Twenty Seventeen: JavaScript coding standards

Reported by: afercia Owned by: afercia
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: has-patch
Focuses: javascript Cc:
PR Number:

Description

All the Twenty Seventeen JavaScript files, excluding third party files, should be checked for proper coding standards.

A few things noticed so far:

  • proper indentation
  • mixed tabs and spaces
  • missing space between if and brace (one occurrence)
  • double spaces

Details, but I guess it's important to show best practices and enforce the usage of coding standards.

Attachments (4)

38752.diff (4.6 KB) - added by sstoqnov 3 years ago.
38752.1.diff (4.6 KB) - added by sstoqnov 3 years ago.
Fix wrong indentation added in 38752.diff
38752.2.diff (13.4 KB) - added by afercia 3 years ago.
38752.3.diff (18.8 KB) - added by afercia 3 years ago.

Download all attachments as: .zip

Change History (11)

#1 @afercia
3 years ago

Also, not sure there's the need for jscs comments like // jscs:disable or // jscs:enable, unless I'm missing something.

@sstoqnov
3 years ago

#2 @sstoqnov
3 years ago

  • Keywords has-patch added; needs-patch removed

Not sure if html5.js is third party file or it should be changed too

@sstoqnov
3 years ago

Fix wrong indentation added in 38752.diff

@afercia
3 years ago

#3 @afercia
3 years ago

Expanding on @sstoqnov patch, I'd avoid code refactoring and just apply some coding standards.

Worth noting couple things, even if they're outside the scope of this ticket:

  • some ternary and if/else seem not necessary to me, especially when setting attributes, maybe code could be simplified a bit
  • in global.js and navigation.js, maybe the functions called on window resize could benefit from abstracting the function that sets the resize timer and use a custom event, as 'wp-window-resized' does in the admin, see wp-admin/js/common.js

@afercia
3 years ago

#4 @afercia
3 years ago

  • Focuses javascript added
  • Owner set to afercia
  • Status changed from new to assigned

Refreshed patch, see:
Spacing for Arrays and Function Calls (Exceptions)
Chained Method Calls

Applied changes where it seemed reasonable to me, any feedback welcome.

#5 follow-up: @davidakennedy
3 years ago

@afercia This looks good to me. Thank you for going through the files and making them better!

I would be open to the changes you noted in 3, but it's probably best in another ticket. These things may also be able to be changed later since they shouldn't change the functionality.

I can work on committing this tomorrow as long as you're good with everything.

#6 in reply to: ↑ 5 @afercia
3 years ago

Replying to davidakennedy:

I can work on committing this tomorrow as long as you're good with everything.

@davidakennedy thanks, sure :)

#7 @davidakennedy
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39221:

Twenty Seventeen: Make sure theme JavaScript follows proper coding standards

Props sstoqnov, afercia.

Fixes #38752.

Note: See TracTickets for help on using tickets.