Make WordPress Core

Opened 6 years ago

Closed 4 months ago

Last modified 4 months ago

#46108 closed enhancement (fixed)

Twenty Nineteen: Include contributing.txt file with Sass compiler instructions

Reported by: kjellr's profile kjellr Owned by: karmatosed's profile karmatosed
Milestone: 6.6 Priority: normal
Severity: normal Version: 5.0.3
Component: Bundled Theme Keywords: has-patch needs-testing commit
Focuses: Cc:

Description

In the GitHub repository, there is a CONTRIBUTING.md document with instructions for how to compile the Sass for Twenty Nineteen. This file was not migrated over to core.

For those working with the theme (both for direct contributions, and for their own purposes), it'd be helpful to bundle instructions for working with the Sass via the included build tool. The attached patch ports over the "Compiling SCSS" section from the document on GitHub, as a new contributing.txt file.

https://github.com/WordPress/twentynineteen/blob/master/CONTRIBUTING.md#compling-scss

Attachments (4)

46108.patch (1.3 KB) - added by kjellr 6 years ago.
46108-1.diff (1.6 KB) - added by nielslange 6 years ago.
46108-2.patch (1.4 KB) - added by kjellr 6 years ago.
46108.3.patch (1.5 KB) - added by sabernhardt 4 months ago.
adds a sentence about running the build command twice, edits "press [return]" to "press the [return] key", and adds a few commas

Download all attachments as: .zip

Change History (18)

@kjellr
6 years ago

This ticket was mentioned in Slack in #docs by kjell. View the logs.


6 years ago

@nielslange
6 years ago

#2 @nielslange
6 years ago

@kjellr I made some minor changes, such as changing my-compter to my-computer Apparently, no one noticed this typo yet. 😁

I've also mentioned running grunt build or grunt watch in the WordPress root folder, to compile the changes from /src into /build.

#3 @kjellr
6 years ago

@nielslange Thanks for fixing that typo. 😄

As for the other note:

Please note that you also need to run grunt build or grunt watch within /my-computer/local-wordpress-install/ to compile the changes from /my-computer/local-wordpress-install/src to /my-computer/local-wordpress-install/build.

This may be confusing for some people, as they may be working from a downloaded copy of the theme. I'd either note that this only applies when working directly with core, or eliminate it (it's more of a general core development convention than a theme workflow note anyway).

#4 @nielslange
6 years ago

Sounds good, @kjellr. Feel free to remove my notes again. 😉

@kjellr
6 years ago

#5 @kjellr
6 years ago

Added 46108-2.patch, which removes that line about grunt-build & grunt-watch.

#6 @karmatosed
4 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to Future Release

#7 @sabernhardt
4 months ago

The npm run build instruction could mention something about (possibly) running the command twice. At least for me, the RTL process can run on the old CSS file before compiling that from Sass files.

Also, when I had a temporary problem with the deprecated node-sass, I thought to propose that we stop editing Twenty Nineteen's Sass files if/when the compiling fails to work. I don't like how much the main stylesheets could change when switching to Dart Sass (without special configuration). Twenty Twenty-One already uses Dart Sass, so that theme should be fine.

#8 @karmatosed
4 months ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 6.6

I am adding this needs a refresh based on that comment and then we can look at getting this in.

@sabernhardt
4 months ago

adds a sentence about running the build command twice, edits "press [return]" to "press the [return] key", and adds a few commas

#9 @sabernhardt
4 months ago

  • Keywords needs-refresh removed
  • Milestone changed from 6.6 to 6.7

This ticket's type is an enhancement, and I do not think that should change.

If the patch is good, though, committing to trunk soon after branching would be nice.

#10 @karmatosed
4 months ago

The approach we are going to take with default themes is to commit until RC and then keep committing. I checked this with @desrosj so we can thankfully keep going - which is amazing. It looks to me like this is a great candidate to do just that.

#11 @karmatosed
4 months ago

  • Keywords commit added
  • Owner set to karmatosed
  • Status changed from new to assigned

#12 @karmatosed
4 months ago

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

In 58451:

Twenty Nineteen: Includes a contributing text file with Sass compiler instructions.

This adds in a useful file to know how to compiled this theme. As there was previously a contributing file with instruction on this in the GitHub repo that was not brought into svn, this fixes that.

Props kjeller, nielslange, sabernahardt.
Fixes #46108.

#13 @karmatosed
4 months ago

Adding a note here to confirm again that default themes are compiled separate so this is absolutely ok to commit during the beta.

#14 @sabernhardt
4 months ago

  • Milestone changed from 6.7 to 6.6
Note: See TracTickets for help on using tickets.