Review: 2501 #2509
Review: 2501 #2509
Conversation
There was a problem hiding this comment.
Thanks, @chrismclarke, really nice to have these changes.
Unlocking the screen orientation by default rather than locking to portrait makes sense. And the signal refactoring is nice.
I will merge this into the PR branch and then run some testing on native platforms and update the PR description.
|
|
||
| constructor( | ||
| private templateService: TemplateService, | ||
| private router: Router | ||
| ) { | ||
| super("TemplateMetadata"); | ||
|
|
||
| // Currently the only watched parameter is for screen orientation, | ||
| // which is only supported on native platforms | ||
| this.enabled = !Capacitor.isNativePlatform(); |
There was a problem hiding this comment.
I can see how setting the enabled state based on a child dependency isn't sensible. But I'm aware that this service adds logic that runs on every route change that currently will only have an effect on deployments that make use of the screen orientation feature. Do you think it would be sensible to do something like run a check of all templates on init to see whether any make use of the parameter_list properties tracked by this service, and only enable it in the case that such a template exists?
Either way, I imagine the performance impact of this service is minimal, so I'm happy to merge as-is.
There was a problem hiding this comment.
Yeah as this has no UI re-rendering effect and given that templates change at most once per few seconds I can't see this really doing much to impact performance.
I agree best way to optimise would be as you've suggested (from the templating). We're already planning to do this for other modules/features, so that if not in use the entire service will be excluded from the build (no need to toggle in the service itself). This should hopefully keep the code a bit simpler.
Template analysis won't know anything about native/web devices, so good to keep those checks in other services, as would opt-in/out for services that don't fully depend on templating such as analytics recording.
PR Checklist
Description
Targets branch of #2501
Main changes:
Review Notes
I've also updated the demo template:
https://docs.google.com/spreadsheets/d/13KnDdmBicPDZaWFNnlIck7WNMLlM02HjD0-_UiqZ4z8/edit?gid=569531329#gid=569531329
For now I've kept enabled on web when not in production (local testing) to see clearly the console logs when orientation changes are being called, let me know if things work as expected to you and we can remove before pushing into production.
I haven't tested on a physical device, but using logs to see if appears to trigger as expected
Git Issues
Closes #
Screenshots/Videos
If useful, provide screenshot or capture to highlight main changes