Page MenuHomePhabricator

[web] Use SettingsSwitcher and Topbar
ClosedPublic

Authored by inka on Jan 23 2023, 9:09 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 11, 8:12 AM
Unknown Object (File)
Fri, Nov 8, 11:37 PM
Unknown Object (File)
Mon, Oct 28, 7:03 AM
Unknown Object (File)
Mon, Oct 28, 7:03 AM
Unknown Object (File)
Mon, Oct 28, 7:03 AM
Unknown Object (File)
Mon, Oct 28, 7:03 AM
Unknown Object (File)
Mon, Oct 28, 7:03 AM
Unknown Object (File)
Mon, Oct 28, 7:03 AM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-2739/move-navigationpanel-from-the-sidebar-to-a-top-bar
The AppSwitcher is now displayed in the topbar. The SettingsSwitcher is displayed as it used to, but I moved it out of the sidebar because of how we use the grid.
Our grid: https://github.com/CommE2E/comm/blob/master/web/style.css#L54
So the SettingsSwitcher has to be a part of app grid area, or the grid layout has to be dynamic.
Changing the way we use the grid - or not using the grid - can be done, but it seems like a big side quest to my community drawer task. So for now I think this is a good enough solution.
It is also consistent between the AppSwitcher and SettingsSwitcher, so I think it's not necessarily bad.

Test Plan

Run web app, check that the SettingSwitcher and AppSwitcher display and work 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, 9:23 AM
inka planned changes to this revision.Jan 23 2023, 9:29 AM

Place the SettingsSwicher in a better place, use the Topbar

inka retitled this revision from [web] Use SettingsSwitcher to [web] Use SettingsSwitcher and the Topbar.Jan 23 2023, 9:58 AM
inka edited the summary of this revision. (Show Details)
inka edited the test plan for this revision. (Show Details)
inka retitled this revision from [web] Use SettingsSwitcher and the Topbar to [web] Use SettingsSwitcher.
inka edited the summary of this revision. (Show Details)
inka edited the test plan for this revision. (Show Details)

Fix filters

inka retitled this revision from [web] Use SettingsSwitcher to [web] Use SettingsSwitcher and Topbar.
inka edited the summary of this revision. (Show Details)
inka edited the test plan for this revision. (Show Details)
inka added inline comments.
web/calendar/filter-panel.css
4 ↗(On Diff #21242)

This makes filters panel in calendar tab display under my topbar, and not partially over it

web/app.react.js
227–262 ↗(On Diff #21242)

I know you just moved most of it to a separate function, but maybe it's a good place to do some refactoring? I'm not a fan of such an imperative approach (empty let, nested if-else etc.). But it's only my subjective opinion, so up to you.

tomek requested changes to this revision.Jan 31 2023, 7:18 AM
tomek added inline comments.
web/app.react.js
237–249 ↗(On Diff #21242)

We can improve the readability by extracting the settings part to the beginning of the function and early exiting.

253–254 ↗(On Diff #21242)

We don't have to introduce arrays

This revision now requires changes to proceed.Jan 31 2023, 7:18 AM
web/app.react.js
227–262 ↗(On Diff #21242)

This is how it's done in most places in the code base. I'm not a huge fun of this approach either, but don't see the point of refactoring just one out of thousands places like this

tomek added inline comments.
web/calendar/filter-panel.css
4 ↗(On Diff #21242)

It would be a lot better if this component was a part of basic layout instead of making its position fixed. It should be possible to position it using e.g. flexbox, but it definitely isn't worth doing now.

This revision is now accepted and ready to land.Feb 7 2023, 7:20 AM