-
Notifications
You must be signed in to change notification settings - Fork 25
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: landscape mode #2501
Feat: landscape mode #2501
Conversation
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.
Functional test passed on the two appetize links.
I was wondering how we're intending this to work with nested templates. If a template contains any nested template that has landscape: true
does it display as landscape itself?
Use case that I'm thinking of is when I create a template my_template
, and many instances of that template using a generator. I can't put any template metadata on the "top" level because the generated templates don't appear in the content list explicitly (unless the metadata from the generator is interpreted as template metadata). So I think it would make sense to put the landscape: true
parameter on my_template
.
It's a good question. I did consider this and by design I believe that landscape mode will be applied only if the top level template has the
I'm not sure I'm understanding this use case correctly, but one option might be to use a top-level template that just consists of a simple wrapper around a child template, and put the |
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.
Thanks for adding @jfmcquade
I like the use of template parameter_list to set the template orientation requirement, although I think I'd be wary of integrating with route params as these are already quite messy and in need of an overhaul.
It also feels like there's quite a lot of interaction going on here more generally, with the template page, container component, processing service, metadata service and orientation service - making it hard to follow the chains of cause and effect.
I think it might be better to refactor to a simpler approach. If (for now) we assume that only top-level templates can impose an orientation change, then I'd probably suggest aomething more like:
-
Use the
templateMetadata
service to subscribe to route changes and retrieve parameter list for active top-level template on change. I think can probably just make a second call to thegetTemplateByName
template service method and we can worry about how better to optimise later. In the future we may also decide to listen to some observable in the service that is tracking all rendered templates and possibly can merge parameters as required, but I'd say keep it more basic for now. -
Use a signal to set a public
parameterList
variable within the templateMetadata service that can be accessed by other services (similar to how we use app and deployment config) -
Add an effect within the
screen-orientation
service that checks for any locked orientation from the current template parameter list and set accordingly.
I'd also suggest using an orientation
template parameter that can be used to force portrait
or landscape
instead of a landscape: true
parameter - just for a bit more clarity.
Thanks, @chrismclarke I think your suggestions sound sensible. On reflection, it's true that we already have the template data, including metadata, when rendering a template, so there's not really any point in passing the data via query params (although I was quite pleased to eventually get the query param updates working). I will refactor to the simpler approach you've suggested. Before I do: @esmeetewinkel what do you think of this suggestion:
As the feature was envisaged as a "landscape mode", I thought that |
The main challenge here is that the template service isn't really keeping track of what is rendered or not, there is no concept of an So there's always going to be a bit of duplication when it comes to identifying the params of the template controlled by the main route. This is also made extra confusing when we also add the idea of standalone templates rendered outside the navigation system. But instead of dealing with all the complexity of the possible case scenarios, I think better to just get things working in a minimal way so that top-level templates can be used to dictate orientation. If needed more fine-grained control there's always the actions that a child template can trigger. |
src/app/shared/components/template/services/template-metadata.service.ts
Outdated
Show resolved
Hide resolved
src/app/shared/components/template/services/template-metadata.service.ts
Outdated
Show resolved
Hide resolved
src/app/shared/services/screen-orientation/screen-orientation.service.ts
Outdated
Show resolved
Hide resolved
src/app/shared/components/template/services/template-metadata.service.ts
Outdated
Show resolved
Hide resolved
src/app/shared/services/screen-orientation/screen-orientation.service.ts
Outdated
Show resolved
Hide resolved
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.
Thanks @jfmcquade , a lot easier to follow now.
I found 2 blocking issues (and a couple minor nits):
-
It looks like you fallback to locking portrait orientation but there might be users who prefer to use landscape. So really the fallback should be to just unlock the orientation and allow user to rotate freely. Adding support for this needs a bit of refactoring to allow for undefined orientation types
-
Orientation set actions get repeatedly called when orientation unchanged which isn't ideal (no idea how expensive an operation this is on device). So there needs to be a check of what is the current locked orientation before attempting to lock
I'll try open a pr for this branch to see if I can resolve so that we can move this forward
(see #2509)
Thanks, @chrismclarke, I've merged your changes and have updated the appetize links. I've also added a new "unlock" value for the I did encounter a potential issue on iOS. It appears that "unlocking" the screen orientation (e.g. when navigating away from a template with orientation set to landscape to one without), the screen does not necessarily return to a state to match the orientation of the actual device. Manually rotating the device does cause the in-app orientation to "catch up". See updated screen recordings in PR description. I don't actually think that this is an issue: I suspect that it wouldn't feel like an issue on an actual device as opposed to an emulator, as after navigating away from a landscape template, rotating the device back to portrait is a natural thing to do. I did try some workarounds, including locking the screen orientation to "natural" before unlocking the screen orientation on iOS ("natural" isn's explained in the documentation, but I believe it should lock the screen orientation to the device's "natural" default, i.e. "portrait" on phones and "landscape" on tablets) However this didn't work as the orientation immediately jumped back to landscape after being momentarily set to portrait. |
Makes sense, although I think in reality this might be misleading to authors who may now think that locking an orientation will keep it locked until unlock (as opposed to unlocking at next navigation). I'm wondering if we might actually want to refine a bit in the future, so that if using actions the orientation stays locked until unlock whereas using template parameters it only stays for that template? What would you think from an authoring perspective @esmeetewinkel ?
Interesting, although again I wouldn't be surprised if the behaviour varies a bit by device anyway, and should still feel reasonably natural to just rotate the device after. Worth getting testing feedback from real devices when in use.
Yeah I was also confused by what was meant by |
I haven't read the full thread, so reading this a bit out-of-context, but what you propose makes sense to me. |
As it stands there's 2 ways to force a template to appear in landscape mode
In both cases the orientation will only stayed locked into landscape until the next time the user navigates to any page where the template does not specify a locked orientation. This ensures that if a user navigates to a template with locked orientation and then comes back the orientation doesn't stay locked. So if you were planning on displaying a series of templates in landscape you would need to make sure to define I think as it stands the action isn't actually any use as you would need to call it from within the template after it has loaded, which doesn't really make much sense. Even if you called the action at the same time you called a navigation go_to action, the orientation will be undone when the template loads unless it has a parameter to lock it. So what I'm suggesting is that we make the action a bit more heavy-handed, where it permanently locks the orientation until an unlock action is called. Of course this gives challenge for the author if they lock the orientation but then the user navigates back and never completes future actions intended to unlock. In this case the author would just need to ensure the initial template is set to |
Part of me is wondering whether it's just better to keep things simple and only use parameter_list to lock a template orientation? |
Yeh, in general I don't see too much of a use for manually setting the orientation via a template action. Apart from, as it happens, our immediate use case, wherein the templates to be displayed in a landscape orientation are authored as fullscreen pop-ups (this means they overlay the header/footer and add a close button, which is desirable). Fullscreen pop-ups don't update the URL in any way, so I was going to try and author a workaround using template actions and update @esmeetewinkel later this evening. Ultimately this could be resolved by having a pop-up system that tracked template metadata (or else updated the top level template name), but we need a solution for the current case immediately |
PR Checklist
Description
Introduces a new template metadata parameter,
orientation
, which can be set on a particular template through theparameter_list
on within the content list. Settingorientation: landscape
for a given template forces the screen orientation to be set to landscape when that template is being displayed. Settingorientation: portrait
for a given template forces the screen orientation to be set to portrait when that template is being displayed. For all other templates, the screen orientation is unlocked, meaning that it should match the user's native screen orientation settings.Testing
As there is no screen orientation API on web, this feature can only be tested on native devices.
See appetize builds:
Git Issues
Closes #2114
Screenshots/Videos
See feature_screen_orientation:
feature_screen_orientation:
Android:
Android.4.mov
iOS:
Note that on iOS, the user may need to physically rotate their device in order for the "unlocked" screen orientation to take effect, for example when navigating away from a template that enforces a landscape orientation.
Appetize:
ios.4.-.appetize.mov
Simluator:
ios.4.-.simulator.mov