Fix app layout issues #1257

zackkrida · 2022-04-11T20:38:56Z

Fixes

Fixes #1214
Fixes #1060
Fixes #1043
Fixes #845

Description

Switches Openverse's frontend to use a truly-sticky header, an actual fixed sidebar, and removes code to hide overflow in parent containers.

Also fixes the mobile header issues by adding w-full to the search input, which overrides the automatic size="20" applied to unsized inputs!

Testing Instructions

Visit the site and observe the filter sidebar, mobile filter popup, and mobile header. If you're on MacOS please try with different scrollbar settings:

You can search 'scrollbar' in system preferences to find those controls. Additionally, please test on mobile devices, the smallest you can find, to make sure that all header elements fit within the viewport without any icons cutting off.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

zackkrida · 2022-04-11T20:40:31Z

@WordPress/openverse-frontend please review this when you can. The one thing I haven't fixed here, that I would love someone to look into, is fixing the header scroll detection code which is used to change the header layout on mobile.

Ready for review!

zackkrida · 2022-04-11T20:41:02Z

src/components/VFilters/VSearchGridFilter.vue

-    class="filters py-8 px-10 min-h-full md:bg-dark-charcoal-06"
-    data-testid="filters-list"
-  >
+  <div class="filters py-8 px-10" data-testid="filters-list">


Styles specific to the search filter sidebar have been moved up in the component tree.

src/layouts/default.vue

# Conflicts: # src/locales/po-files/openverse.pot

src/components/VHeader/VSidebarContent.vue

obulat

I have triple scroll bar: two vertical and one horizontal :(

…d into layout-fixes

sarayourfriend · 2022-04-15T04:52:30Z

Something here has broken the body scroll lock for the filters modal on mobile (but not the search type switcher 🤔)

sarayourfriend

This is exciting!

Couple of issues I'm noticing:

Filter sidebar has a scrollbar on desktop widths when the translation banner is visible
When both banners are open the header is so big it obscures almost half the screen on mobile devices
There is horizontal scroll on smaller viewports in the filter and search type modals
On smaller viewports the header wraps, I'm not sure what the right solution there is, but maybe the search type switcher button isn't truncating as expected? 🤔
The mobile global audio player is behaving quite strangely. It's not actually meant to show on the single result page anyway. Some of this may be caused by mysterious horizontal scrolling on the audio single results page but other aspects of it (like the global audio player not sticking to the bottom of the screen) are unique to this branch.

sarayourfriend · 2022-04-15T04:54:32Z

src/layouts/default.vue

    watch([isMainContentScrolled], ([isMainContentScrolled]) => {
      isHeaderScrolled.value = isMainContentScrolled
    })
-    watch([mainContentY], ([mainContentY]) => {
-      scrollY.value = mainContentY
+    watch([windowY], ([windowY]) => {
+      scrollY.value = windowY


These can probably be computeds if you want to update them.

zackkrida · 2022-04-15T11:33:59Z

Great review @sarayourfriend, thank you! I think I can fix this stuff 😄

obulat · 2022-05-10T07:44:01Z

I'm confused why those xs screenshots of the attribution tabs changed width 🤔

I think that it was caused by the main being wider than the screen width, and it was overflowing horizontally. At least, when I set main to use class="w-screen", all of the component started fitting into the xs screen, and it seems nothing else broke.

src/layouts/default.vue

obulat · 2022-05-10T08:33:37Z

The changes in the latest PRs:

the SearchBar search button was set not to shrink. This stops the search button from being too narrow on small screens.
main width is set to 100vh (Tailwind w-screen class). This stops the app from being too wide for the xs screens. This fixes the media reuse visual regression tests, and also makes sure that everything fits into the screen on xs breakpoint.
Mobile header vertical padding is set to the smaller py-3 value to match the mockups.

I think this PR is ready to be merged 🚀

zackkrida · 2022-05-10T14:09:27Z

@obulat

main width is set to 100vh (Tailwind w-screen class). This stops the app from being too wide for the xs screens. This fixes the media reuse visual regression tests, and also makes sure that everything fits into the screen on xs breakpoint.

w-screen does not account for the width of a visible scrollbar, so it adds horizontal scrolling when scrollbars are set to visible, like on Windows.

Trying w-full instead, ~~I believe the tests still pass.~~ I'll do w-screen md:w-full. As far as I know there are no mobile browsers which display vertical scrollbars. There's likely a child misbehaving that's causing the expansion on small sizes that I'll also investigate if time permits.

sarayourfriend · 2022-05-10T15:41:52Z

As far as I know there are no mobile browsers which display vertical scrollbars. There's likely a child misbehaving that's causing the expansion on small sizes that I'll also investigate if time permits.

Note that that's just a stopgap. I regularly use Openverse under the md breakpoint on my laptop (with visible scrollbars).

zackkrida · 2022-05-10T16:53:44Z

@sarayourfriend a full, non stop-gap fix for this is applied, and the Playwright tests are passing.

krysal

Such a huge improvement here. I love that the header doesn't make two lines when is scrolled on mobile anymore! :)

#1212 is still an unresolved issue, but that could be addressed apart. Great work here!

zackkrida · 2022-05-10T19:04:33Z

Ah you're right @krysal #1212 isn't related to this PR. Thanks for the review!

Fix app layout issues

c2fe971

zackkrida requested a review from a team as a code owner April 11, 2022 20:38

zackkrida requested review from krysal and sarayourfriend April 11, 2022 20:38

zackkrida marked this pull request as draft April 11, 2022 20:39

openverse-bot added 🕹 aspect: interface Concerns end-users' experience with the software 🛠 goal: fix Bug fix 🟨 priority: medium Not blocking but should be addressed soon labels Apr 11, 2022

zackkrida commented Apr 11, 2022

View reviewed changes

Fix sidebar height class

3be1239

zackkrida commented Apr 11, 2022

View reviewed changes

src/layouts/default.vue Outdated Show resolved Hide resolved

zackkrida and others added 3 commits April 14, 2022 17:48

Fix app layout issues

40b83b9

Fix sidebar height class

0c10b27

Merge branch 'main' into really_fix_load_more

23b992f

# Conflicts: # src/locales/po-files/openverse.pot

obulat force-pushed the layout-fixes branch from 3be1239 to 23b992f Compare April 14, 2022 14:50

obulat reviewed Apr 14, 2022

View reviewed changes

src/components/VHeader/VSidebarContent.vue Show resolved Hide resolved

obulat reviewed Apr 14, 2022

View reviewed changes

zackkrida added 5 commits April 14, 2022 16:31

Merge branch 'layout-fixes' of github.com:WordPress/openverse-fronten…

b32edcf

…d into layout-fixes

merge main

03251a4

Cleanup for rtl and remove horizontal scrollbar

0cb7d57

Window scroll detection and proper input width

087bea5

Fix search count alignment

e34a05b

zackkrida marked this pull request as ready for review April 14, 2022 21:17

sarayourfriend reviewed Apr 15, 2022

View reviewed changes

obulat added 3 commits May 10, 2022 08:59

Update mobile header paddings (make them smaller)

189beef

Extract dismissTranslationBanner util function

40d7614

Update snapshots

dd28c44

obulat added 2 commits May 10, 2022 10:44

Update POT file

fab148c

Update snapshots

ca3198a

obulat reviewed May 10, 2022

View reviewed changes

src/layouts/default.vue Show resolved Hide resolved

obulat added 2 commits May 10, 2022 11:19

Revert updates to xs media reuse snapshot

91094d2

Remove unnecessary header-height CSS variable

2cb96db

obulat self-requested a review May 10, 2022 11:55

obulat approved these changes May 10, 2022

View reviewed changes

Switch the "main" element to w-full instead of w-screen

c61aa76

Use viewport width on mobile; switch to full-width above

3bb8f23

zackkrida added 4 commits May 10, 2022 12:34

Fix related image width

c0599cd

Merge branch 'main' into layout-fixes

f0359e3

Merge main

3221063

.pot file update

cf4044e

zackkrida requested a review from sarayourfriend May 10, 2022 17:00

krysal approved these changes May 10, 2022

View reviewed changes

zackkrida merged commit 19b9864 into main May 10, 2022

zackkrida deleted the layout-fixes branch May 10, 2022 19:04

fcoveram mentioned this pull request May 23, 2022

Pages menu is cut off in mobile landscape view #1212

Closed

sarayourfriend mentioned this pull request Sep 6, 2022

[BUG] Unable to set viewport width and capture fullpage screenshot microsoft/playwright#13339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix app layout issues #1257

Fix app layout issues #1257

zackkrida commented Apr 11, 2022 •

edited

Loading

zackkrida commented Apr 11, 2022 •

edited

Loading

zackkrida Apr 11, 2022

obulat left a comment

sarayourfriend commented Apr 15, 2022

sarayourfriend left a comment

sarayourfriend Apr 15, 2022

zackkrida commented Apr 15, 2022

obulat commented May 10, 2022

obulat commented May 10, 2022

zackkrida commented May 10, 2022 •

edited

Loading

sarayourfriend commented May 10, 2022

zackkrida commented May 10, 2022 •

edited

Loading

krysal left a comment

zackkrida commented May 10, 2022

Fix app layout issues #1257

Fix app layout issues #1257

Conversation

zackkrida commented Apr 11, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

zackkrida commented Apr 11, 2022 • edited Loading

zackkrida Apr 11, 2022

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

sarayourfriend commented Apr 15, 2022

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Apr 15, 2022

Choose a reason for hiding this comment

zackkrida commented Apr 15, 2022

obulat commented May 10, 2022

obulat commented May 10, 2022

zackkrida commented May 10, 2022 • edited Loading

sarayourfriend commented May 10, 2022

zackkrida commented May 10, 2022 • edited Loading

krysal left a comment

Choose a reason for hiding this comment

zackkrida commented May 10, 2022

zackkrida commented Apr 11, 2022 •

edited

Loading

zackkrida commented Apr 11, 2022 •

edited

Loading

zackkrida commented May 10, 2022 •

edited

Loading

zackkrida commented May 10, 2022 •

edited

Loading