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(@capacitor/screen-orientation): Orientation 'landscape' lock uses current held landscape #2040

Open
wants to merge 6 commits into
base: 5.x
Choose a base branch
from

Conversation

codepip55
Copy link

Fixes #2039

@codepip55 codepip55 changed the title Add separate statements for landscape option feat(@capacitor/screen-orientation): Orientation 'landscape' lock uses current held landscape Feb 27, 2024
@brian-weasner
Copy link
Contributor

@codepip55 This has conflicts now that the code in #2022 has been merged.

Copy link
Contributor

@brian-weasner brian-weasner left a comment

Choose a reason for hiding this comment

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

Comment: Merge conflicts accidentally overwrote changes that I made as a part of #2022

@codepip55
Copy link
Author

I think I've fixed everything now, let me know if I missed something :)

@brian-weasner
Copy link
Contributor

@IT-MikeS I know you just helped get #2022 merged, is there a way we can get this in soon as well so that they can roll out in the same releases for cap v5 and cap v6?

@IT-MikeS IT-MikeS self-assigned this Apr 22, 2024
@codepip55
Copy link
Author

Is there a timeline for when we can expect this to be merged?

@shiv19
Copy link

shiv19 commented May 26, 2024

The linting warnings from CI are unrelated to the changes made in this PR.

@ericmulder
Copy link

Any news on this pull request?

@codepip55
Copy link
Author

Any updates on this PR?

@jansgescheit
Copy link

Any updates on this PR @brian-weasner @IT-MikeS ? it's been lying around for a long time now and many people are waiting for it

@ericmulder
Copy link

Hi @brian-weasner and @IT-MikeS, would it be possible to merge this?

@brian-weasner
Copy link
Contributor

Thanks for the @, as this fell off of my radar after my bugfix ticket got merged.

@ericmulder , @jansgescheit,
NOTE: I'm not on the capacitor team so I don't have merge privileges.

This issue just came about around the same time I was fixing #2022. Because of that I've been communicating with this ticket to be sure that it's changes wouldn't overwrite the changes I made.

Since #2022 has been merged into 5.x branch and subsequently into Capacitor v5 and v6, although there is no mention of it in the CHANGELOG.md in either v5 or v6 branches, there is a workaround that can be used. Simply get the current orientation, check to see if it contains the word "landscape" and if so lock it to that orientation.

The following code has not been tested:

const currentOrientation = await ScreenOrientation.orientation();
if (currentOrientation.includes('landscape')) {
    await ScreenOrientation.lock({orientation: currentOrientation]);
}

I still agree that this should be merged because this brings about nice changes. @IT-MikeS Is there anything blocking this from getting merged?

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.

6 participants