Open OpuRockey opened 2 years ago
This theme seems to have been based on Underscores -- but the license in package.json was changed from GPL-2.0-or-later to MIT -- why was that change made?
I have checked commit history and MIT added since beginning and I don't know why. We can change it back to GPL-2.0-or-later
.
Underscores also has a /style.css in the root directory of the theme that is built from the /sass/ folder in the theme. It looks like you've rearchitected the Sass to be be nested several directories down in the /assets/src/sass/ and output to the /assets/build/css/ folders, rather than the WordPress-standard /style.css file in the root. Why?
As best practise, we need to generate different CSS files for different template so that we can avoid loading CSS which is not needed to different files. For example, homepage CSS may not be needed on single or archive pages. It helps to improve performance. Because of that we are not using default style.css
file and kept all final build CSS in separate folder like single place to manage source and build files.
It seems the main.css and other generated css files are compressed at the highest degree, saving some bits of data with the tradeoff of maintenance and comprehension. Was this an intentional choice?
Final build CSS files only used in front-end and never going to use for development purpose. We have source files for development.
Why was the functions.php from underscores rewritten to a much more complex autoloader system?
We are doing development using OOP concepts. We are using PHP autoloader functionality to avoid using including class files manually. Also, because of autoloader, class files only loaded when it going to use. PHP autoloader implementation is not complex. Also having separate files for different functionalities will keep functions.php file code minimal.
Could you explain how you weighted the cost/benefit of building wrapper methods for WordPress core functionality such as registering styles? It feels like a needless bit of complexity that makes it more difficult to ack through the codebase and finds where resources are registered, and I'm not sure I understand the advantage when it's only used three or four times.
We are creating JS-FILE-NAME.assets.php
and this file will have list of depedencies and version info and using it while registering/enqueuing JS files. For CSS, files, we are using file's modified time as version. To reduce repetitive code of fetching that information, created different methods.
Is there a reason for the theme not including a theme.json file for the block editor to be able to use to guide font sizes, colors, and the like?
No reason. It's remaining to add theme.json
file.
For how constants are named -- something ending in _TEMP_DIR feels confusing when it's used to refer to a "Template Directory" rather than a directory for temporary files -- minor stuff, but more of a remark that it may want to be reevaluated for naming patterns.
Agree on this. It should be TEMPLATE
and not TEMP
to avoid confusion.
Note: This is base theme so it might feel that few functionalities may not require abstraction or wrapper. For example, for autoloading or additional methods to enqueue JS/CSS files. But it's required when more features and functionalities implemented in theme. Also, if theme features are basic and required minimal development then we can remove codes which may not needed.
@RahiDroid @manishsongirkar Request you to share your thoughts on above questions.
cc: @OpuRockey
@OpuRockey
@chandrapatel @manishsongirkar Requesting your 👀 on this, please.
@nitun @OpuRockey Created an issue for adding the theme.json
file in the skeleton (6th point). For the 1st one, I did reach out to a few FE folks, but nobody seems to have an idea about what that change was made. It was introduced in this commit.
Hi @RahiDroid you can fix and merge that license issue.
I believe we can dual-license the code if we want. Since code in package.json
is not GPL licensed(written by other contributors) hence we can keep that part MIT licensed.
Dual License Example - https://github.com/arnavyc/dual-licensed-mit-apache Related Discussion on Gutenberg (for reading purposes) - https://github.com/WordPress/gutenberg/issues/29483
cc/ @RahiDroid @nitun @manishsongirkar @OpuRockey
👋 So a couple thoughts on the above --
On 2 -- While I can certainly understand that there can be an advantage of sharding out styles into multiple files for the load time of an initial page view, that wasn't my question.
Specifically for the styles that are global and going to be loaded on every page, why not just use the existing WordPress standard of the theme's style.css
for the output? If there are sub-styles that need to be included conditionally, then by all means, I can understand using secondary sheets in a sub-folder so as to not clutter up the root directory -- however it feels like the reasoning provided was "Why we split css assets" not "Why the primary css file isn't using WordPress's default location" as variances from core WordPress functionality can be disorienting for other developers that may step in for future maintenance.
On 3 -- Is there a reason to not provide a documented place in the configuration / build scripts where this could be modified to generate CSS output more in line with WordPress's coding styles? Especially for future maintenance where developers that aren't running a full build environment of the theme may need to rapidly identify where styles are coming from and make modifications in the WP Customizer's Custom CSS editor, having the build tools generate human-readable output can be incredibly useful, so I'd strongly encourage a documented option for how to modify the compression level of the build tools used. Also, I believe that the perf benefits of minifying are far less on http/2 due to the hpack compression and such?
On 4 -- While an autoloader system may not be "complex" to many, to be clear I had initially described it as "more complex" -- which I believe we can all agree compared to keeping the bulk of the functionality in the functions.php
file as WordPress core themes do, it is additional complexity. My concern here is for accessibility to less technical support and account managers somewhat comfortable with skimming code, as well as consistency with WordPress core patterns when dozens or hundreds of sites with themes from any number of developers are being used.
So my point is that it is a trade-off, and that trade-off needs to be acknowledged. There is added complexity here, and it's just a question of whether that trade-off is merited, and whether the goals and wins from it (a more minimal functions.php file, avoiding explicitly including files, sharding of functionality into smaller files) are worth the tradeoffs from consistency across sites, added complexity, and the maintenance load going forward.
On 5 -- I still have concerns regarding the added complexity here, as well as being unsure as to the (largely theoretical) perf impacts of pulling in extra files (which may likely be mitigated by opcache) versus explicitly stating the parameters in the code where it's used, but thank you for the explanation of your rationale here.
A couple other questions I'd had --
Blank_Theme
instead?init theme
functionality swaps all instances of Blank to the new theme's name, there are very few places left in the generated theme where one could look at and understand that it was compiled from blank-theme
. It may be worth adding a paragraph in the generated theme's readme and style.css header (kinda where it still keeps the "Based on Underscore" comment) that it also clarifies the lineage that it was based on Blank as well, for understanding and better identifying the origin of some of the code and choices made.edit: undid the # before the numbers above, apologies for xref'ing other issues!
Hi @georgestephanis 👋
Appreciate you taking the time and sharing your thoughts on this.
On 2 -- I still think that having all my stylesheets placed in the same place is more convenient than having to toggle between the assets folder and the style.css
file in the root during the development. Splitting styles between the style.css
file and other stylesheets in the assets folder might also cause a developer to think that the styles present in the style.css
file is all that is there and there are no other stylesheets.
On 3 -- If you're talking about the customizer's custom CSS editor, you should be able to track back the source of a ruleset using the developer tools and then override in the editor.
(as it says, it's coming from the action-list.scss
file)
Or, if you were referring to the Theme file editor
, then yes, I totally agree with you, having a human-readable file would be helpful. But then, it's a very specific use case. However, that's a good starting point for the discussion and we could definitely evaluate the need and incorporate the option you suggested! 👍
On 4 -- Default/core themes like TwentyTwentyOne are kept very lean and plain, however, while developing complex themes, things get crazy cluttered and messy quite easily as the codebase grows, and navigating through the codebase becomes a lot difficult. I believe you're hinting toward the autoloader functionality being complex and not something that one would easily be able to understand at a first look; I agree with you.
I think the trade-off is worth it, the segregation of the classes isn't very different from what's being done in the WordPress core themes as well. I think the only difference is the autoloading functionality here, which adds the complexity you mentioned but also provides really good benefits.
Other thoughts
Thank you so much for all the suggestions and feedback @georgestephanis ! 🙂
Hello
I got some questions regarding the blank theme from the partner.
ack
through the codebase and finds where resources are registered, and I'm not sure I understand the advantage when it's only used three or four times.Thanks
CC: @chandrapatel