-
Notifications
You must be signed in to change notification settings - Fork 0
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
Improve view engines and their integration in the Admin #70
Open
veve40
wants to merge
15
commits into
main
Choose a base branch
from
feature/admin-view
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ckage Uses a container admin/view set with Mustache engine only for admin package. Resolves issues with default view from different contexts. Add custom Admin TemplateRoute extending App TemplateRoute with the overwriting view container. Overwrite model/dependencies to set the admin/view container.
mcaskill
changed the title
Pulling feature/admin-view into main
Add dedicated view engine for the Admin
Oct 13, 2022
mcaskill
requested changes
Oct 13, 2022
packages/admin/src/Charcoal/Admin/Property/Display/StatusDisplay.php
Outdated
Show resolved
Hide resolved
packages/admin/src/Charcoal/Admin/ServiceProvider/AdminServiceProvider.php
Outdated
Show resolved
Hide resolved
packages/admin/src/Charcoal/Admin/ServiceProvider/AdminServiceProvider.php
Outdated
Show resolved
Hide resolved
Replace email/factory for admin/email/factory in admin module in order to force using mustache view engine for emails. Offer the possibilty to keep using the default view engine (ie front) even for sending email from backend.
mcaskill
reviewed
Nov 1, 2022
packages/admin/src/Charcoal/Admin/ServiceProvider/AdminServiceProvider.php
Outdated
Show resolved
Hide resolved
Use an email/dependencies container overwritten in admin module to use specific view engine
…il/provider and Admin Template Route. Fix the calls of render and renderTemplate in admin section
mcaskill
reviewed
Nov 4, 2022
mcaskill
requested changes
Nov 4, 2022
packages/admin/src/Charcoal/Admin/Widget/FormGroup/NestedWidgetFormGroup.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Chauncey McAskill <[email protected]>
…ring engine decider Rename file decider and string decider for file engine decider and string engine decider for ViewAggregator
mcaskill
requested changes
Nov 8, 2022
mcaskill
approved these changes
Nov 9, 2022
mcaskill
changed the title
Add dedicated view engine for the Admin
Improve view engines and their integration in the Admin
Nov 9, 2022
The 'view' service is provided by the parent class `AbstractWidget`.
mcaskill
force-pushed
the
feature/admin-view
branch
from
November 9, 2022 15:01
9564218
to
5d75d30
Compare
Remove unused and duplicate imports.
A viewable object does not have a context parameter.
Changed: - Moved logic irrelevant to JSON serialization outside of try/catch. - Added `JsonException` handling to `PhpEngine` to match `TwigEngine`. - Sorted namespace imports.
mcaskill
approved these changes
Nov 9, 2022
mcaskill
requested changes
Nov 1, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reread this pull request and I think there's a few improvements we can make:
- Rename
decider
toresolver
. - Replace the JSON re-encoding of the context as an associative array in
PhpEngine
andTwigEngine
with a better approach that does not lose access to objects.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The View package's primary API elements are the
render()
(for template files:path/to /view.mustache
) andrenderTemplate()
(for template strings:{{ person.name }}
) methods. Their somewhat vague names caused widespread misuse because Mustache (for a long time the default and only supported template engine) provides a single methodrender()
for rendering both. This caused a lot of trouble during the initial integration of Twig 3 (#44).This pull request attempts to resolve most of the leftover issues in supporting both Mustache and Twig and their mixed use within the Admin (for features such as content blocks).
Breaking Changes
Replaced all inconsistent use of
render()
andrenderTemplate()
with the appropriate function call for the given circumstance.If a renderable value was previously a template file path (
path/to /view.mustache
) it may need to be converted to a template include tag ({{> path/to/view }}
) and vice versa.For example, renderable elements in Selectize (SelectizeRenderer) now require a template string.
In classes such as
NestedWidgetFormGroup
andFormGroupWidget
:The
description
andnotes
are localized and rendered in their setter methods instead of their getter methods. This avoids recursion with templating engines such as Twig which require the view data to be serialized unlike Mustache which accepts objects.In the View package, the in-memory cache of registered dynamic templates now static to share templates across view loader classes.
View
View Aggregator
Added a new view adapter:
ViewAggregator
. This is now the default adapter and uses Mustache, PHP, and Twig (in that order).The aggregator is used to register several engines to either use a specific engine later on or to dynamically resolve which engine to render any given template file or template string.
The
get()
method allows you to retrieve a view adapter for an engine by its name. This is useful in situations where a single portion may need rendering in a specific engine.The
using()
method allows you to change the default engine to use by its name. This is useful in situations where the overall kind of templates being rendered changes dramatically.Note that the default engine is only used if the templating file or tag syntax can not be determined during the dynamic resolution process.
The deciders are functions that receive the given template, context, available engines, and the default engine, and handle which engine to should be used. For example:
Changed
$context
parameter inPhpEngine
andTwigEngine
. Classes now throw an exception instead rendering with a bad value (false
).setEngine()
from private to protected inAbstractEngine
.loader()
from protected to public inAbstractEngine
.isTemplateString()
andfindTemplateFile()
from protected to public inAbstractLoader
.resolveDynamicTemplate()
inAbstractLoader
.Admin
Changed
MessageDisplay
,StatusDisplay
since it is provided byAbstractProperty
.view
service in classes whose parent class handles the dependency.view
service fromAdminServiceProvider
, replaced withViewAggregator
.Email
Changed
Email
dependencies fromemail/factory
, asemail/dependencies
, inEmailServiceProvider
. Similar tomodel/dependencies
inModelServiceProvider
.