Page MenuHomePhabricator

[web] Add possibility for the navigation-panel to be horizontal

Authored by inka on Jan 23 2023, 8:43 AM.
Referenced Files
Unknown Object (File)
Thu, Mar 16, 7:51 PM
Unknown Object (File)
Sat, Feb 25, 3:40 AM
Unknown Object (File)
Fri, Feb 24, 9:42 PM
Unknown Object (File)
Feb 21 2023, 2:21 AM
Unknown Object (File)
Feb 21 2023, 1:59 AM
Unknown Object (File)
Feb 17 2023, 11:07 AM



When AppSwitcher (that uses NavigationPanel underneath) is used in the Topbar it'll need to be horizontal. The tabs will also have to display differently. But the SettingSwitcher (that also uses NavigationPanel underneath) is supposed to stay as it is.

Test Plan

Pass horizontal={true} to NavigationPanel.Container in AppSwitcher and check that it displayes correctly.

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

inka requested review of this revision.Jan 23 2023, 8:57 AM

JS is ok, deferring CSS to someone else

51 ↗(On Diff #21198)

I'm not sure about this one. Will this class name be changed when switching the horizontal prop?

This revision is now accepted and ready to land.Jan 24 2023, 6:19 AM
inka added inline comments.
51 ↗(On Diff #21198)

(eslint says to do it, and even if it's not needed, I don't think it hurts)
We change the css we use depending on the horizontal prop, so the css class referenced by this changes (Theoretically, because I don't expect the horizontal prop to be changing for a given navigation panel). Is that not right?

This revision now requires review to proceed.Jan 24 2023, 6:45 AM
51 ↗(On Diff #21198)

eslint says to do it

This is convincing enough, thanks ;)
Now I recall that this dependency might make sense if the CSS classnames can be hashed/obfuscated by bundler/transpiler or sth, but not sure how it works here.

[...] so the css class referenced by this changes

The class itself changes, but this memo depends only on its name, which should be constant (unless it's hashed), this is why I asked initially ;)

Theoretically, because I don't expect the horizontal prop to be changing for a given navigation panel)

Yes, but we shouldn't depend on the "immutability by design" of this prop - it isn't supposed to be mutated now, but it can be in the future and nothing's blocking it

I'm not sure if this is the best solution. Usually in cases like this, we could introduce a new class e.g. horizontal and check it in selectors: we would have span.chatBadge selector and span.chatBadge.horizontal. This allows splitting the code which depends on the orientation from the code that doesn't. On the other hand current solution is still maintainable, so we can consider keeping it.

62–63 ↗(On Diff #21198)

We have some consts for it.

This revision is now accepted and ready to land.Jan 25 2023, 8:59 AM
62–63 ↗(On Diff #21198)

Actually we don't, but this is the only place in the code where we use these values, so @tomek and I agreed to leave it like this. Especially that I was just moving this code from somewhere else.