Page MenuHomePhabricator

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

Authored by inka on Jan 23 2023, 8:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 7:44 AM
Unknown Object (File)
Feb 20 2024, 7:51 PM
Unknown Object (File)
Feb 18 2024, 1:48 AM
Unknown Object (File)
Feb 18 2024, 12:13 AM
Unknown Object (File)
Feb 17 2024, 11:35 PM
Unknown Object (File)
Feb 17 2024, 9:57 PM
Unknown Object (File)
Feb 17 2024, 9:17 PM
Unknown Object (File)
Feb 17 2024, 7:55 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-2739/move-navigationpanel-from-the-sidebar-to-a-top-bar
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

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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

JS is ok, deferring CSS to someone else

web/sidebar/navigation-panel.react.js
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.
web/sidebar/navigation-panel.react.js
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
web/sidebar/navigation-panel.react.js
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.

web/topbar/topbar.css
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
web/topbar/topbar.css
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.