Page MenuHomePhorge

Fix overflowing of AphrontSideNavFilterView on small screens and tidy up nav bar styles
AcceptedPublic

Authored by bekay on May 4 2024, 13:21.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jun 20, 13:53
Unknown Object (File)
Thu, Jun 20, 07:50
Unknown Object (File)
Sat, Jun 15, 12:48
Unknown Object (File)
Wed, Jun 12, 23:25
F2224483: D25619-frontpage-after.png
Tue, Jun 11, 08:50
F2224481: D25619-frontpage-before.png
Tue, Jun 11, 08:50
Unknown Object (File)
Sun, Jun 9, 14:06
Unknown Object (File)
Sun, Jun 9, 14:06

Details

Summary

Switch the two column layout of AphrontSideNavFilterView (used in pretty much most views) from table to flex display. The former CSS rendering could lead to overflowing of the entire page on small screens if very wide unbreaking content is displayed (example described here: T15809).

While I was at it I have found some unused code. The CSS file phabricator-nav-view.css consisted almost only of .phabricator-side-menu-home class selectors. This is an ancient class (there at least since 2013) not used anymore:

https://we.phorge.it/source/phorge/browse/master/src/applications/directory/controller/PhabricatorDirectoryController.php;919bd4a03499305093d8882952a9dd3dac1c55a9$169

So I have removed this CSS component and saved the few still used declarations to the main phui-basic-nav-view.css.

There was some unused code in AphrontSideNavFilterView.php too.

Fixes T15809

Test Plan

Look at this video to understand the problem:

F2178277

Go in a Task comment and mention a task with a very-long title. Have phd enabled to generate feeds. Visit homepage with feeds:

See that you cannot reproduce anymore.

Browse pages with the right nav bar with many devices and look if the layout looks fine and not broken.

Tested the homepage, scaling from mobile to desktop, using:

  • Firefox 124 64bit on GNU/Linux (snap)
  • Chromium 124 64bit on GNU/Linux (snap)
  • ...

Tested other elements, scaling from mobile to desktop, still working correctly:

  • popup dialogs
  • right floating menu
  • top curtain
    • notifications
    • Conpherence chats
    • unresolved setup issues
    • search omnibox

Diff Detail

Repository
rP Phorge
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1300
Build 1300: arc lint + arc unit

Event Timeline

bekay requested review of this revision.May 4 2024, 13:21

All described here with screenshot (and now video): T15809

But with my test you can just go to https://we.phorge.it with your mobile and see it for yourself.

Ah, yes thank you for the video. I should have clarified but was asking for screenies of the result after this change. The changes look reasonable but I’m not a regular css user

In D25619#17360, @speck wrote:

Ah, yes thank you for the video. I should have clarified but was asking for screenies of the result after this change. The changes look reasonable but I’m not a regular css user

The site looks the same of course just no overflowing 😋 long unbreaking elements are just cut off. Hopefully no big chance. But I think it should be checked out on some test enviromnents of busy reviewers.

valerio.bozzolan edited the test plan for this revision. (Show Details)

Maybe "feature" but note the left sidebar, is not anymore connected to the end of the viewport:

BeforeAfter
Maniphest list before.png (643×1 px, 78 KB)
Maniphest list after.png (643×1 px, 78 KB)
  • Fixes non stretching nav bar if content is absolute or does not fill entire page height

Maybe "feature" but note the left sidebar, is not anymore connected to the end of the viewport:

BeforeAfter
Maniphest list before.png (643×1 px, 78 KB)
Maniphest list after.png (643×1 px, 78 KB)

I have fixed this.

aklapper subscribed.

Hi bekay, thanks for looking into this, very appreciated!

Speaking as a reviewer, splitting into two patches would have been slightly easier (but speaking as a developer that's more fiddling and stacking is hard in arc so... your approach is understandable). :)

Removal of unused variables in src/view/layout/AphrontSideNavFilterView.php and basically webroot/rsrc/css/aphront/phabricator-nav-view.css makes sense - I verified via grep that variable aren't used within the PHP file and that the CSS file is not loaded/mentioned anywhere else.

Which leaves us with reviewing the changes in webroot/rsrc/css/application/base/standard-page-view.css and webroot/rsrc/css/phui/phui-basic-nav-view.css.
I tested on the frontpage and pages like http://phorge.localhost/diffusion/ with two varying widths: table-cell gets replaced by flex in the browser's developer tools; the rendered layout does stay the same.

Funny enough I do not see a change in behavior in Chromium in the Feed for mentioned tasks with a long title. Might be Chromium behaving differently here.

But I do see a welcome improvement on the frontpage:
Before:

D25619-frontpage-before.png (767×584 px, 95 KB)

After:
D25619-frontpage-after.png (767×584 px, 106 KB)

So all in all this looks good to me.
I'm going to accept personally for now.
If noone else who feels like a skilled CSS reviewer shows up for another review to also set Accept as O1: Blessed Committers, then I'll be happy to also accept as O1 in a week or such...

This revision is now accepted and ready to land.Fri, Jun 21, 13:28
aklapper retitled this revision from Fixes overflowing of AphrontSideNavFilterView on small screens and tidies up nav bar styles to Fix overflowing of AphrontSideNavFilterView on small screens and tidy up nav bar styles.Fri, Jun 21, 13:28