feat(app): add focus trap to main nav and QuestionsSidebar; add small fixes to lockScroll by grzegorzpokorski · Pull Request #489 · typeofweb-org/devfaq

grzegorzpokorski · 2023-01-06T15:36:01Z

dodałem 'focus trap' do menu głównego oraz sidebara,
dodałem nowy hook useIsAboveBreakpoint, który zwraca true kiedy szerokość viewportu przekroczy przekazaną wartość, w przeciwnym wypadku zwraca false,
dodałem zmianę w pageScroll.ts - dodawanie padding-right po ukryciu scrollbarów, działa tylko dla użytkowników przeglądarki Safari, ponieważ na ten moment nie wspiera scrollbar-gutter z css.

PS. prace nad rozwiązanymi problemami prowadzone były również w PR #484, jednak nie chciało mi się już rozwiązywać konfliktów i postanowiłem dodać nowy PR 😆

vercel · 2023-01-06T15:36:04Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
devfaq	✅ Ready (Inspect)	Visit Preview	Jan 11, 2023 at 7:33PM (UTC)

github-actions

Device	URL
mobile	https://devfaq-ck1f6nfzr-typeofweb.vercel.app

Not what you expected? Are your scores flaky? GitHub runners could be the cause.
Try running on Foo instead

github-actions · 2023-01-06T15:38:03Z

📦 Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action 🤖

⚠️ Global Bundle Size Increased

Page	Size (compressed)
`global`	`83.31 KB` (🟡 +2 B)

Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

typeofweb · 2023-01-06T17:10:56Z

-export const unlockScroll = ({ mobileOnly }: { mobileOnly: boolean }) => {
-	document.body.style.paddingRight = "";
+export const unlockScroll = ({ mobileOnly, preventLayoutShift }: PropsType) => {
+	if (preventLayoutShift) {


Po co jest ta opcja i kiedy chcemy ustawić ją na false? Wydaje mi się, że nigdy.

W przypadku kiedy otwieramy menu mobilne dodawanie padding-right do body nic nie zmienia, ponieważ i tak nie widać tego przesunięcia strony spowodowanego usunięciem scrollbarów. Podobnie sprawa ma się w przypadku QuestionsSidebara w widoku mobilnym - cała strona jest zakryta po jego otwarciu

Natomiast dodanie tego odstępu może powodować w niektórych sytuacjach problemy np:

otwieram menu mobilne:

przekręcam widok na widok horyzontalny i oczom ukazuje się niepotrzebny odstęp od scrollbara:

Ta dodatkowa opcja została dodana właśnie z myślą o tym przypadku.

Faktem jest również to, że dodawanie odstępu od prawej po ukryciu scrollbara ma znaczenie tylko w przeglądarkach, gdzie on jest cały czas widoczny, czyli najczęściej na desktopowych urządzeniach / przeglądarkach. Na smartfonie tego niepożądanego efektu nie dostrzeżemy.

To nie prościej byłoby po prostu usunąć ten padding przy obracaniu ekranu? :)

Opisany wyżej przypadek można by było wyeliminować również w ten sposób, że w zależności od wartości isAboweBreakpoint, czyli krótko mówiąc w momencie przechodzenia z widoku mobilnego na desktopowy i odwrotnie jeśli menu było by otwarte to należało by dodać ten odstęp lub go zabrać, lecz problem jest taki że, jak scrollbary są widoczne to można sobie obliczyć ile tego odstępu dodać, ale jak chcę przywrócić ten odstęp i mieć nadal zablokowaną możliwość scrollowania to nie ma skąd tego odstępu obliczyć. Gdyby tak na przykład dać w useEffect warunek, że jeśli np sidebar jest otwarty i isAboreBreakpoint === true to dodaj scrollbary i usuń odstęp to by załatwiło sprawę, ale wpływ na to czy maja być widoczne scrollbay mają również inne elementy np menu i modale i dodatkowo to wszystko się jeszcze zmienia w zależności od szerokości viewportu. To często generowałoby konflikty i doprowadzało by do sytuacji, że np menu mobilne ma widoczne scrollbary, a nie powinno.

Krótko mówiąc ta dodatkowa właściwość eliminuje konflikty w bardzo specyficznych sytuacjach.

Najprościej by było, gdyby sidebar i menu było zamykane przy przejściu z widoku mobilnego na desktopowy i ponowne przejście w drugą stronę (deskop -> mobile) nie powodowało odtworzenia poprzedniego stanu tj. na mobile menu było otwarte czy nie tylko zawsze takie przejście zamykało by czy to menu czy sidebara.

To nie prościej byłoby po prostu usunąć ten padding przy obracaniu ekranu? :)

załóżmy że usunę ten parametr prewentLayoutShift z pageScroll.ts, a do HeaderNavigation dodam coś takiego co pozornie rozwiązuje problem:

useEffect(() => { if (isAboveBreakpoint) { document.body.style.paddingRight = ""; } }, [isAboveBreakpoint]);

i teraz:

w widoku mobilnym otwieram menu mobilne

zmieniam widok na desktopowy

otwieram modal np. 'dodaj pytanie'

wracam do widoku mobilnego

wracam do widoku desktopowego

...strona nie posiada scrollbarów, nie posiada odstępu - zamykam modal i w trakcie tego zamykania, strona przesuwa się...

a to jeden z przypadków, nie mówiąc już o tym, że mając otwarte menu mobilne w widoku desktopowym ciągle działa useEffect w tle i modyfikuje style body

#484 (comment) spełnienie wymagania z tego komentarza, generuje najwięcej problemów

@mmiszy tak z ciekawości zerknąłem, jak to jest rozwiązane na allegro.pl. W wersji mobilnej, jak otworzę sidebar 'FIltry i kategorie' to do body jest dodawana klasa css z overflow: hidden; , ale jak z otwatym sidebarem zmienie szerokość viewportu do rozmiaru desktopowego to nadal mam zablokowaną możliwość scrollowania 😁

Zawsze jakieś kompromisy...

ale przedstawię zaraz jeszcze jedna wersję chyba ostateczną, jeśli nie ona to już sam nie wiem...

Dobra, próbowałem przygotować inną wersję, w której stan czy menu jest otwarte, czy zamknięte oraz funkcje zmieniające ten stan znajdowałyby się w UIProvider, ale kod zaczyna wyglądać strasznie, a każdy element: menu, modale, sidebar ma funkcjonalność opisywaną praktycznie w sposób okropny, bo trzeba np. w modalu brać pod uwagę czy menu jest otwarte / zamknięte tak samo z sidebarem....nie dobrze to wygląda

Ja mam taką propozycję:

zostaje taka wersja jaka jest - wiem są kompromisy, ale nie da się ich w 'sensowny' sposób przeskoczyć (przez sensowny mam na myśli nie opisywać, jak dany element ma się zachować dla każdego szczególnego przypadku w zależności od stanu innego elementu na stronie),

idziemy na kompromis i każde przejście z mobile na desktop bądź na odwrót powoduje 'wyzerowanie' stanu tego elementu do ustawień początkowych np. otwarte menu mobilne przy przejściu do widoku desktopowego jest zamykane i po ponownym powrocie do widoku mobilnego nadal będzie zamknięte, w ten sposób można bardzo łatwo uniknąć konfliktów.

Na chwilę obecną przetestowałem już tyle wersji, podpatrywałem, jak sobie z tym radzą inne serwisy i nie da się tego zrobić inaczej. Zawsze jest jakiś kompromis, chyba że ręcznie rozpiszesz i obsłużysz wszystkie możliwe kombinacje otwarcia i zamknięcia menu, modali, sidebarów. Współczuję jednak jak postanowisz dodać kolejny element...

github-actions

Device	URL
mobile	https://devfaq-26gsrrx2x-typeofweb.vercel.app

Not what you expected? Are your scores flaky? GitHub runners could be the cause.
Try running on Foo instead

github-actions

Device	URL
mobile	https://devfaq-hjb4llv27-typeofweb.vercel.app

Not what you expected? Are your scores flaky? GitHub runners could be the cause.
Try running on Foo instead

grzegorzpokorski · 2023-01-07T13:19:05Z

Uprościłem kod dodając do html klasę css z nową właściwością scrollbar-gutter, dzięki czemu pilnowanie, aby treść strony się nie przesuwała podczas usuwania scrollbarów, zostaje przerzucone na przeglądarkę. Stare rozwiązanie zostało zachowane jako fallback dla Safari, które nie wspiera tej nowej własności cssa.

Właściwość scrollbar-gutter wydaje się być jak najbardziej uzasadniona, ponieważ rozmiar scrollbara jest inny w różnych przeglądarkach, a w przypadku kiedy go usuniemy i ponownie chcielibyśmy dodać margines od prawej, aby zapobiec przesunięciu strony to nie ma na jakiej podstawie wyznaczyć ile ten margines ma wynosić, ponieważ nie mamy scrollbara, wiec naturalne wydaje się, że ten problem trzeba scedować na przeglądarkę.

typeofweb · 2023-01-11T16:48:34Z

+type PropsType = {
+	mobileOnly: boolean;
+};


Wcześniej w typie PropsType było więcej propsów, a teraz uznałem, że skoro wykorzystuje w 2 fukcjach takie same propsy to postanowiłem to pozostawić. Rozumiem że lepiej wrócić do:

export const lockScroll = ({ mobileOnly }: { mobileOnly: boolean }) => {

Co-authored-by: Michał Miszczyszyn <michal@mmiszy.pl>

…nto fix-main-menu

github-actions

Device	URL
mobile	https://devfaq-qpu3k31i5-typeofweb.vercel.app

Not what you expected? Are your scores flaky? GitHub runners could be the cause.
Try running on Foo instead

Co-authored-by: Michał Miszczyszyn <michal@mmiszy.pl>

github-actions

Device	URL
mobile	https://devfaq-rgo4ccyhz-typeofweb.vercel.app

Not what you expected? Are your scores flaky? GitHub runners could be the cause.
Try running on Foo instead

github-actions

Device	URL
mobile	https://devfaq-3sbz2w1i8-typeofweb.vercel.app

Not what you expected? Are your scores flaky? GitHub runners could be the cause.
Try running on Foo instead

sonarqubecloud · 2023-01-11T19:32:24Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

github-actions

Device	URL
mobile	https://devfaq-1mafmwms7-typeofweb.vercel.app

Not what you expected? Are your scores flaky? GitHub runners could be the cause.
Try running on Foo instead

… fixes to lockScroll (#489) * feat(app): add 'useIsAboveBreakpoint' hook * refactor(app): refactor 'pageScroll' utility file * feat(app): add focus lock in HeaderNavigation * fix(app): changes in BaseModal to reflect changes in pageScroll * feat(app): add focus trap in 'QuestionsSidebar' * fix(app): fix hamburger button position * fix scrollbar gutter * Update apps/app/src/utils/pageScroll.ts Co-authored-by: Michał Miszczyszyn <michal@mmiszy.pl> * Update apps/app/src/utils/pageScroll.ts Co-authored-by: Michał Miszczyszyn <michal@mmiszy.pl> * refactor(app): change types declarations in 'pageScroll' * refactor(app): refactor styles related to scrollbar on page * Update apps/app/src/components/Header/HeaderNavigation.tsx Co-authored-by: Michał Miszczyszyn <michal@mmiszy.pl> * refactor(app): refactor 'HeaderNavigation' * fix(app): fix typo Co-authored-by: Michał Miszczyszyn <michal@mmiszy.pl>

grzegorzpokorski added 6 commits January 6, 2023 14:51

feat(app): add 'useIsAboveBreakpoint' hook

949265a

refactor(app): refactor 'pageScroll' utility file

720e88a

feat(app): add focus lock in HeaderNavigation

df9e37f

fix(app): changes in BaseModal to reflect changes in pageScroll

99424ba

feat(app): add focus trap in 'QuestionsSidebar'

1235b02

fix(app): fix hamburger button position

83b2b5e

grzegorzpokorski requested a review from typeofweb as a code owner January 6, 2023 15:36

github-actions Bot reviewed Jan 6, 2023

View reviewed changes

grzegorzpokorski mentioned this pull request Jan 6, 2023

feat(app): add useIsAboveBreakpoint hook; focus trap to main nav and sidebar #484

Closed

typeofweb requested changes Jan 6, 2023

View reviewed changes

grzegorzpokorski marked this pull request as draft January 6, 2023 19:08

fix scrollbar gutter

8bfa39d

vercel Bot deployed to Preview January 7, 2023 01:47 View deployment

github-actions Bot reviewed Jan 7, 2023

View reviewed changes

Merge branch 'develop' into fix-main-menu

180069d

vercel Bot deployed to Preview January 7, 2023 01:56 View deployment

github-actions Bot reviewed Jan 7, 2023

View reviewed changes

grzegorzpokorski marked this pull request as ready for review January 7, 2023 01:59

typeofweb requested changes Jan 11, 2023

View reviewed changes

grzegorzpokorski and others added 4 commits January 11, 2023 18:27

Merge branch 'develop' into fix-main-menu

663e61d

Update apps/app/src/utils/pageScroll.ts

4d078a4

Co-authored-by: Michał Miszczyszyn <michal@mmiszy.pl>

Update apps/app/src/utils/pageScroll.ts

4740f50

Co-authored-by: Michał Miszczyszyn <michal@mmiszy.pl>

Merge branch 'fix-main-menu' of https://github.com/typeofweb/devfaq i…

3992cb1

…nto fix-main-menu

vercel Bot deployed to Preview January 11, 2023 17:34 View deployment

github-actions Bot reviewed Jan 11, 2023

View reviewed changes

grzegorzpokorski added 2 commits January 11, 2023 18:49

refactor(app): change types declarations in 'pageScroll'

4964b6d

refactor(app): refactor styles related to scrollbar on page

405bfd4

Update apps/app/src/components/Header/HeaderNavigation.tsx

851ff3a

Co-authored-by: Michał Miszczyszyn <michal@mmiszy.pl>

vercel Bot deployed to Preview January 11, 2023 18:01 View deployment

github-actions Bot reviewed Jan 11, 2023

View reviewed changes

refactor(app): refactor 'HeaderNavigation'

9683e77

vercel Bot deployed to Preview January 11, 2023 18:05 View deployment

github-actions Bot reviewed Jan 11, 2023

View reviewed changes

fix(app): fix typo

485a761

vercel Bot deployed to Preview January 11, 2023 19:33 View deployment

github-actions Bot reviewed Jan 11, 2023

View reviewed changes

typeofweb approved these changes Jan 11, 2023

View reviewed changes

typeofweb merged commit 081bc8f into develop Jan 11, 2023

typeofweb deleted the fix-main-menu branch January 11, 2023 22:21

Conversation

grzegorzpokorski commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel Bot commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Next.js Bundle Analysis

⚠️ Global Bundle Size Increased

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grzegorzpokorski Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grzegorzpokorski Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grzegorzpokorski Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grzegorzpokorski Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

grzegorzpokorski commented Jan 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jan 11, 2023

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

grzegorzpokorski commented Jan 6, 2023 •

edited

Loading

vercel Bot commented Jan 6, 2023 •

edited

Loading

github-actions Bot commented Jan 6, 2023 •

edited

Loading

grzegorzpokorski Jan 6, 2023 •

edited

Loading

grzegorzpokorski Jan 6, 2023 •

edited

Loading

grzegorzpokorski Jan 6, 2023 •

edited

Loading

grzegorzpokorski Jan 6, 2023 •

edited

Loading

grzegorzpokorski commented Jan 7, 2023 •

edited

Loading