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.

Sign up for GitHub

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

Jump to bottom

Resolve invalid non-full-screen menus on some mobile #7912

Merged
merged 2 commits into from Apr 9, 2023
Merged

Resolve invalid non-full-screen menus on some mobile #7912

merged 2 commits into from Apr 9, 2023

Conversation

Soltus
Copy link
Contributor

@Soltus Soltus commented Apr 7, 2023

Passed the following tests:

  • MIUI14 webview 108
  • OriginOS3 webview 101
  • Chromium Desktop Chromium 113
  • Chrome Desktop Chromium 86

@88250 88250 requested a review from Vanessa219 April 7, 2023 03:07
@Soltus Soltus marked this pull request as draft April 7, 2023 06:03
@Soltus Soltus marked this pull request as ready for review April 7, 2023 06:11
Vanessa219
Vanessa219 reviewed Apr 7, 2023
View reviewed changes
app/src/menus/Menu.ts
// this.element.style.top = "50vh";
// 解决方案二，需要联动上面的 item.style.top
this.element.style.position = "sticky";
this.element.style.height = "500px";
Copy link
Member

Choose a reason for hiding this comment

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

这个值不能写死吧，异常的原因是由于部分机型 window.innerHeight / 2 不等于 50vh 造成的么？

Copy link
Contributor Author

@Soltus Soltus Apr 7, 2023 •
edited

Choose a reason for hiding this comment

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

应该很少有屏幕高度不足的手机，我认为方案二的体验会更好。不写死可以使用方案一。
异常的原因未知，仅在特定机型上出现

Copy link
Member

Choose a reason for hiding this comment

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

横屏可能会有问题，看来只能使用方案一了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我测试一下横屏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

横屏没有问题
}N9M(~0(R1)_E`PZDBL{67S
伺服也没问题
image

Copy link
Member

@Vanessa219 Vanessa219 Apr 9, 2023 •
edited

Choose a reason for hiding this comment

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

有的横屏没有 500px 吧？我直接换成你的第一种方案吧，谢谢。

@Soltus Soltus marked this pull request as draft April 7, 2023 15:39
@Soltus Soltus marked this pull request as ready for review April 7, 2023 15:47
@Soltus
Copy link
Contributor Author

Soltus commented Apr 7, 2023

要不加个动效吧，目前弹出来很突兀

@Vanessa219 Vanessa219 merged commit 5fd53ac into siyuan-note:dev Apr 9, 2023
@Vanessa219 Vanessa219 changed the title Resolve invalid non-full-screen menus on the mobile Resolve invalid non-full-screen menus on the some mobile Apr 9, 2023
@Vanessa219 Vanessa219 added this to the 2.8.4 milestone Apr 9, 2023
@Soltus
Copy link
Contributor Author

Soltus commented Apr 9, 2023 via email

Vanessa219 added a commit that referenced this pull request Apr 9, 2023
@Vanessa219
🎨 fix #7912
1bc838a
@Vanessa219
Copy link
Member

已提交第一种方案，这里没有有问题的机型，还麻烦再帮忙测试一下。多谢。

@Vanessa219 Vanessa219 mentioned this pull request Apr 9, 2023
Closed
@Soltus
Copy link
Contributor Author

Soltus commented Apr 9, 2023

已提交第一种方案，这里没有有问题的机型，还麻烦再帮忙测试一下。多谢。

正式版看一下吧，应该没问题了

@Soltus Soltus deleted the Soltus-patch-1 branch April 9, 2023 09:45
@88250 88250 changed the title Resolve invalid non-full-screen menus on the some mobile Resolve invalid non-full-screen menus on some mobile Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vanessa219 Vanessa219 Vanessa219 left review comments

Assignees

@Vanessa219 Vanessa219

@Soltus Soltus

Labels
Enhancement
Projects
None yet
Milestone
2.8.4
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Soltus @Vanessa219