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(ios): add iOS orientation #3365

Closed
wants to merge 0 commits into from
Closed

feat(ios): add iOS orientation #3365

wants to merge 0 commits into from

Conversation

lisicold
Copy link

@lisicold lisicold commented Jul 7, 2023

Pre-PR Checklist

  • I added/updated relevant documentation.
  • I followed the Convention Commit guideline with maximum 72 characters to submit commit message.
  • I squashed the repeated code commits.
  • I signed the [CLA].
  • I added/updated test cases to check the change I am making.
  • All existing and new tests are passing.

@lisicold lisicold requested a review from ozonelmy as a code owner July 7, 2023 08:25
@tencent-adm
Copy link
Member

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


v_zxingli seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions bot added framework: ios size: s Denotes a PR that changes 10-99 lines labels Jul 7, 2023
@hippy-service
Copy link

hippy-service bot commented Jul 7, 2023

Hi, @lisicold. Thanks for your PR! 👏

🏷️ You can leave a comment in this PR with #help tag when you need help (e.g. some status checks run failed due to internal issue), admin team members will help asap.

@hippy-service
Copy link

hippy-service bot commented Jul 7, 2023

After a quick scan, I have approved workflow to run.

🏷️ New commits in this PR would not be tested automatically until this pull request is reviewed by our collaborators.
🏷️ No need to worry about the status of merge_guard and [gh] pull request merge guard / merge_guard (pull_request_target) checks, once this pull request is met merge requirements, it will be automatically converted to successful status.

@github-actions github-actions bot added size: xs Denotes a PR that changes 0-9 lines and removed size: s Denotes a PR that changes 10-99 lines labels Jul 14, 2023
Copy link
Collaborator

@wwwcg wwwcg left a comment

Choose a reason for hiding this comment

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

可能需修改对齐,等待Android端提交后一同合入

static UIInterfaceOrientation interfaceOrientation = UIInterfaceOrientationPortrait;/// 异常情况给一个默认的UIDeviceOrientationPortrait
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
[[NSNotificationCenter defaultCenter] addObserverForName:UIDeviceOrientationDidChangeNotification
Copy link
Collaborator

Choose a reason for hiding this comment

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

该PR有几点问题:
1、此处监听通知并不会及时通知到 js侧,仅是native层刷新。真正hippyExportedDimensions依赖的是UIApplicationDidChangeStatusBarOrientationNotification这个通知。
2、UIDeviceOrientationDidChangeNotification是设备方向变化通知,绑定给前端的orientation变量含义是有歧义的,是“设备方向”?还是“页面布局方向”?,最佳做法可能是提供两个变量进行区分。
3、首次获取方向的代码需优化,避免出现Magic Number等,类似这种(interfaceOrientation - deviceOrientation == 0)

@hippy-service
Copy link

hippy-service bot commented Jul 17, 2023

Hi, @lisicold, I noticed that our reviewers requested changes to this pull request.
When you're done, click the Re-request review button in the right sidebar(shown below) to notify the reviewer.
Re-request review button in the right sidebar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework: ios size: xs Denotes a PR that changes 0-9 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants