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

Повсеместное использование --safe-area-inset-top #2414

Merged
merged 5 commits into from
Apr 25, 2022
Merged

Повсеместное использование --safe-area-inset-top #2414

merged 5 commits into from
Apr 25, 2022

Conversation

inomdzhon
Copy link
Contributor

@inomdzhon inomdzhon commented Apr 19, 2022 •
edited
Loading

В частности поправил, что на Android игнорировалось использование --safe-area-inset-top в следующих компонентах:

Проверил на эмуляторе

safari

О значении по умолчанию для iOS

Для iOS в --safe-area-inset-top в :root выставлялось значение по умолчанию в 20px (прикрепляю код ниже)

VKUI/src/styles/constants.css

Lines 34 to 35 in ca0a2da

/* iOS insets */
--safe-area-inset-top: 20px;

Из-за этого для других платформ приходилось обнулять это значение точечно (прикрепляю пример ниже)

VKUI/src/components/PanelHeader/PanelHeader.css

Lines 155 to 160 in ca0a2da

@supports not (padding-top: env(safe-area-inset-top)) {
.PanelHeader--android,
.PanelHeader--vkcom {
--safe-area-inset-top: 0px;
}
}

Чтобы избежать этого, сделал так, что значение по умолчанию выставлялось только для iOS (ссылка на коммит)

DevTools Safari

safari

DevTools Android

android

UPD
В рамках этого PR, также поправил следующие issues:

@inomdzhon inomdzhon requested a review from a team as a code owner April 19, 2022 09:03
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 19, 2022 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0628334:

Sandbox Source
VKUI - default example Configuration

@github-actions
Copy link
Contributor

github-actions bot commented Apr 19, 2022 •
edited
Loading

size-limit report 📦

Path Size
JS 75.64 KB (0%)
JS, unstable 25.81 KB (0%)
CSS 42.43 KB (-0.09% 🔽)
CSS, unstable 1015 B (0%)

@github-actions
Copy link
Contributor

👀 Styleguide deployed

See the styleguide for this PR at https://vkcom.github.io/VKUI/pull/2414/

@github-actions
Copy link
Contributor

github-actions bot commented Apr 19, 2022 •
edited
Loading

Code coverage

lines3669 / 469278.19%
statements3738 / 478278.16%
functions823 / 102880.05%
branches3045 / 434470.09%
branchesTrue0 / 0100.00%

Generated by 🚫 dangerJS against 0628334

@stoope
Copy link
Contributor

stoope commented Apr 19, 2022

При landscape ориентации не сломается?)

stoope
stoope reviewed Apr 19, 2022
View reviewed changes
src/styles/constants.css Outdated
*/
@supports (-webkit-touch-callout: none) {
:root {
--safe-area-inset-top: 20px;
Copy link
Contributor

Choose a reason for hiding this comment

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

А зачем мы ставим 20 на iOS? Есть какие то кейсы где в safe-area-inset-top не проставляется высота? А если iphone без челки?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Вот на айфонах без чёлки как раз 20. Ещё на айфонах без чёлки может быть старая iOS, где будут отсутствовать нужные нам переменные окружения.

Copy link
Contributor

Choose a reason for hiding this comment

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

Мне не понятно почему для IOS выставляется 20 а для остальных платформ перебивается в 0. Это какой то баг был или особенность мини аппок?

Copy link
Contributor Author

@inomdzhon inomdzhon Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Тут #2414 (comment) я глянул, что с 20px напрямую в Safari выглядит так себе

Если фикс для мини-апп, то можно же через VKWebAppUpdateConfig сетить эти 20px, не?

Copy link
Contributor Author

@inomdzhon inomdzhon Apr 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@stoope Игорь рассказал мне в оффлайне, что значение по умолчанию в 20px было необходимо для версий iOS без поддержки CSS Custom Properties. Это позволяло полифилить --safe-area-inset-top

:root {
  --safe-area-inset-top: 20px
}

.ComponentA {
  top: 20px
  top: var(--safe-area-inset-top);
}

Пришли к тому, что можно удалить это значение, т.к. прошло достаточно много времени, чтобы считать, что iOS с такой версий уже почти нет.

Запушил коммит.

PS: 20px это высота статус бара в iOS без челки

@inomdzhon
Copy link
Contributor Author

inomdzhon commented Apr 19, 2022 •
edited
Loading

При landscape ориентации не сломается?)

Воу, однако...

Но гляжу, что отключена ротация в приле))

demo

@inomdzhon
Copy link
Contributor Author

inomdzhon commented Apr 20, 2022 •
edited
Loading

На iOS тоже отключена ротация приложения.

Глянул в браузере, там, где поддерживается safe-area-inset-top, он 0px.

Ещё интересная особенность, что система сама переключает значения в зависимости от portrait и landscape (прикрепил демо ниже)

safe inset on portrait

Рис. 1. 0px на portrait

safe inset on landscape

Рис. 2. 47px landscape

Нюанс про 20px по умолчанию

Но если не будет поддерживаться, то эти 20px смещают контент, что в portrait, что в landscape. @fedorov-xyz правильно понимаю, что выставили 20px в контексте бага в Мини Апп?

@inomdzhon
Copy link
Contributor Author

inomdzhon commented Apr 20, 2022 •
edited
Loading

Ещё обнаружил небольшой баг #2422 – как раз связано с тем, о чём выше писал. А именно, что на landscape константы safe-area-inset-left и safe-area-inset-right больше 0px.

Мог бы поправить сразу в этой ветке) @stoope что думаешь?

@inomdzhon inomdzhon mentioned this pull request Apr 20, 2022
Open
@stoope
Copy link
Contributor

stoope commented Apr 21, 2022 •
edited
Loading

Ещё обнаружил небольшой баг #2422 – как раз связано с тем, о чём выше писал. А именно, что на landscape константы safe-area-inset-left и safe-area-inset-right больше 0px.

Мог бы поправить сразу в этой ветке) @stoope что думаешь?

Ага, давай починим. А что если использовать position: sticky у PanelHeader?

@eolme
Copy link
Contributor

eolme commented Apr 21, 2022

Со sticky не все так просто, легко получить дополнительные проблемы, когда его что-то может перекрыть, либо он перестанет адекватно позиционироваться из-за особенностей его работы

@inomdzhon inomdzhon force-pushed the 2183-fix-ModalPage-safe-area-inset-in-android branch from bafedbb to 230b4dc Compare April 22, 2022 05:25
@inomdzhon
Copy link
Contributor Author

А что если использовать position: sticky у PanelHeader? @stoope

Со sticky не все так просто, легко получить дополнительные проблемы, когда его что-то может перекрыть, либо он перестанет адекватно позиционироваться из-за особенностей его работы @eolme

Это да, sticky по щелчку не влепить
В рамках другой задачи можно будет поизучать

@inomdzhon inomdzhon merged commit 4eb437a into master Apr 25, 2022
@inomdzhon inomdzhon deleted the 2183-fix-ModalPage-safe-area-inset-in-android branch April 25, 2022 06:19
@inomdzhon inomdzhon mentioned this pull request May 11, 2022
Merged
@mendrew mendrew mentioned this pull request Feb 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedorov-xyz fedorov-xyz fedorov-xyz left review comments

@stoope stoope stoope approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@inomdzhon @stoope @eolme @fedorov-xyz