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.

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

feat: split frontend asset generation into separate steps for more extensibility #3446

Merged
merged 2 commits into from
Jun 19, 2022

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented Jun 2, 2022

Changes proposed in this pull request:

This PR splits out the logic within frontend asset generation into separate protected methods on the class to allow for more simple extending of asset generation.

For example, this is particularly useful for FoF Nightmode. Instead of completely duplicating logic found in this class, it can now override methods in order to add its own CSS compiler, and will no longer need to manually force commit assets.

It also means that future changes to core are less likely to impact extensions hooking into this class (which caused FriendsOfFlarum/nightmode#60, for example).

You can see the impact this has in this diff: https://github.com/FriendsOfFlarum/nightmode/compare/72a75dec1ed85be8a3ec0c0442a87a6688157e84..dw/flarum-1.4-rewrite

Reviewers should focus on:

I'm not totally familiar with this area of core, so am unsure if this will have additional unforeseen (by me) impacts. Local testing with the Nightmode branch above does not raise any issues for me.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

@davwheat davwheat self-assigned this Jun 2, 2022
@davwheat davwheat added this to the 1.4 milestone Jun 2, 2022
@SychO9 SychO9 modified the milestones: 1.3.1, 1.4 Jun 7, 2022
@davwheat davwheat merged commit 1d949a3 into main Jun 19, 2022
@davwheat davwheat deleted the dw/frontend-asset-accessibility-overhaul branch June 19, 2022 21:58
@luceos
Copy link
Member

luceos commented Sep 16, 2022

@davwheat I cannot see your diff, is this something we need to introduce in nightmode now? I.. don't see the benefit of what was merged here or the necessary rewrite for nightmode.

@davwheat
Copy link
Member Author

I think the necessary nightmode changes may have already been merged.

I'm not totally sure what the intention was after this got merged anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants