Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sage 10 Cleanup #2171

Merged
merged 5 commits into from
May 1, 2019
Merged

Sage 10 Cleanup #2171

merged 5 commits into from
May 1, 2019

Conversation

Log1x
Copy link
Member

@Log1x Log1x commented May 1, 2019 •
edited
Loading

This PR turned into a little more than I originally anticipated, but I will follow some more opinionated changes with my reasoning and we can go from there.

Changelog

Enhancements

  • Enable preflight by default when not in production.
    • Having it disabled by default is just asking for confusion, specifically with the auto-creation of the Acorn cache directory for compiled Blade templates.
  • Revert @php and @endphp to @php()
    • Contrary to popular belief, @php() still works perfectly fine. The only caveat is it can not be mixed with @endphp in the same template. With the addition of Composers, not only should using @php be avoided at all costs, but @php(...) works perfectly fine when multi-lined. @php() is considerably more aesthetic, easier to read, cleaner, and doesn't break syntax highlighting in editors such as VSCode.
  • Remove excessive linebreaks from helpers.php.
  • Make config linebreaking uniform.
    • Single linebreak before and after docblocks– consistent with Laravel.
  • Add missing space to ! conditionals to stay consistent with PSR-1/2.
  • Add aside markup to sidebar and wrap it in @hasSection.
  • Add a document wrapper with an ID of app.
    • Not only necessary for ARIA compliance (role="document") – but useful to have in place for Vue, etc.
  • Add ARIA roles to the document, main, aside, header, nav, and footer wrappers.
  • Change the entry-meta conditional in content-search.blade.php to an @includeWhen.
  • Add some sane linebreaks throughout the views and codebase to increase readability.
  • Add (missing) docblocks to the example Title.php composer.
  • Add WP Blade Check to alert users of publicly viewable Blade files.
  • Bump version to 10.0.0 in package.json and style.css.
  • Bump roots/sage-installer to ^1.6.
  • Change PHP requirement to ^7.1.3 to match Acorn (and Laravel).
  • Remove deprecated qwp6t/acorn repository from composer.json.
  • Add some sane optimizations to composer.json (courtesy of Laravel).
  • Update the theme structure and sponsors in README.md.
  • Set trim_trailing_whitespace to false for *.md files to allow linebreaks in markdown files.

Bugfixes

  • Add missing form.search view.
  • Unescape $title where used.

Log1x added 2 commits May 1, 2019 02:01
@Log1x
Add missing search blade
da723af 
Enable preflight by default
Bump version to 10.0.0 in package.json and style.css
Change PHP requirement to 7.1.3
Upgrade roots/sage-installer
Remove deprecated qwp6t/acorn repository from composer.json
Add some sane optimizations to composer.json
Remove excessive linebreaks from helpers.php
Make config linebreaking uniform
Add missing space to ! conditionals to stay consistent with PSR-1/2
Update the file structure in README.md and add the new sponsor
Revert to shorthand @php()
Add aside markup to sidebar and wrap it in @hasSection
Add a document wrapper
Add ARIA roles to the document, main, aside, header, nav, and footer wrappers
Change entry-meta conditional in content-search.blade.php to an @includeWhen
Add some sane linebreaks throughout the views and codebase to increase readability
Add docblocks to the example Title.php composer
@Log1x
Add WP Blade Check
5c72cc9
@Log1x Log1x mentioned this pull request May 1, 2019
Merged
6 tasks
@QWp6t
Copy link
Member

QWp6t commented May 1, 2019

I'm not sure if I'm on board with wrapping the header and footer elements. ARIA role for main and aside are redundant since those are already their default roles, and when direct descendants of the body, the default roles for header and footer are banner and contentinfo, respectively. The document role is unnecessary since web pages are treated as documents by default. It would only be necessary if the parent element had a different role, such as a widget or an application.

@Log1x
Copy link
Member Author

Log1x commented May 1, 2019

That's fair. I'll scrap them.

@QWp6t
Copy link
Member

QWp6t commented May 1, 2019

I see a mixture of {!! get_something() !!} vs @php(something()).

In resources/views/partials/comments.blade.php, you have:

<li class="next">{!! get_next_comments_link(__('Newer comments &rarr;', 'sage')) !!}</li>

In resources/views/layouts/app.blade.php, you have:

<html @php(language_attributes())>

I think we should be consistent here. I'm cool with either one, but I slightly lean toward {!! ... !!} because it's explicit that there is output and that the output isn't being filtered by Blade.

QWp6t
QWp6t reviewed May 1, 2019
View reviewed changes
config/app.php Outdated
'preflight' => false,

'preflight' => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a compromise, I would say to have preflight set to WP_ENV !== 'production' or something so that by default it's not enabled in production.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess one afterthought I have on this is the default location of the cache being in uploads – perhaps this might be problematic for those who are just pushing updates to their base theme and not pushing the new cache up?

If so, would putting cache in resources maybe be more sane?

QWp6t
QWp6t reviewed May 1, 2019
View reviewed changes
composer.json Outdated Show resolved Hide resolved
@Log1x
Revert language_attributes() to get_language_attributes()
e3888bb 
Remove roots/wp-blade-check until we decide on where to put it
Only enable preflight when not in production
@Log1x
Copy link
Member Author

Log1x commented May 1, 2019

I think we should be consistent here. I'm cool with either one, but I slightly lean toward {!! ... !!} because it's explicit that there is output and that the output isn't being filtered by Blade.

I agree on the consistency. I did it in this case out of somewhat personal appeal as we are stuck with using @php(body_class()) below it (since get_body_class() returns as an array) and it looked a bit cleaner / smaller footprint. It was also what was used prior to getting rid of @php() (I think I'm the one who PR'd it).

Nonetheless I reverted it for now.

@retlehs retlehs merged commit a3fa3e0 into roots:10.0.0-dev May 1, 2019
@retlehs
Copy link
Member

retlehs commented May 1, 2019

thanks for all this @Log1x! merging this now since it fixes the missing search view

we can address any other concerns in a new branch/PR (like the cache folder)

@Log1x
Copy link
Member Author

Log1x commented May 1, 2019

No problem. Sounds good.

@Log1x Log1x mentioned this pull request Jul 8, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QWp6t QWp6t QWp6t left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Log1x @QWp6t @retlehs