Page MenuHomePhabricator

[web] Simplify `AppSwitcher` component and style tabs when active/on hover
ClosedPublic

Authored by atul on Mar 7 2022, 8:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 4, 11:29 AM
Unknown Object (File)
Sun, Dec 29, 9:33 AM
Unknown Object (File)
Sun, Dec 29, 9:33 AM
Unknown Object (File)
Sun, Dec 29, 9:33 AM
Unknown Object (File)
Sun, Dec 29, 9:33 AM
Unknown Object (File)
Sun, Dec 29, 9:33 AM
Unknown Object (File)
Sun, Dec 29, 9:33 AM
Unknown Object (File)
Sun, Dec 29, 9:33 AM

Details

Summary

Simplify AppSwitcher layout by avoiding the use of unordered list (required some workarounds, overriding default styling, etc).

By simplifying the layout, we're able to properly style the app tabs when active/on hover.

Here's what it looks like:

Note: Yes I realize there's an "and" in the diff title. Intentionally breaking this rule and including both the refactor and styling in the same diff since they're closely related and I think it'll be easier to review. If I were to make a second diff with the active/hover styling it would literally just be adding:

div.appTab:hover p,
div.appTab:hover svg,
div.activeTab p,
div.activeTab svg {
  color: var(--fg);
  stroke: var(--fg);
  fill: var(--fg);
}
Test Plan

NA

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul requested review of this revision.Mar 7 2022, 8:52 PM
atul planned changes to this revision.Mar 8 2022, 8:23 AM

Looks like AppSwitcher has changed a lot since, I'll rebase this and redo it after everything has landed.

In D3363#90833, @atul wrote:

Looks like AppSwitcher has changed a lot since, I'll rebase this and redo it after everything has landed.

Thanks! I think it's easier to redo this refactoring than D3341. I think it makes sense to add a dependency here.

I know this diff is larger than usual, but I think makes sense to keep all these changes in one place since they're interrelated.

I could force this into smaller diffs, but I don't think that would be particularly helpful.

Updated video:

web/icons/selection.json
1034 ↗(On Diff #10458)
web/sidebar/app-switcher.react.js
61 ↗(On Diff #10458)

Makes the whole "rectangle" clickable instead of just the text

web/sidebar/left-layout-aside.css
4 ↗(On Diff #10458)

This variable doesn't exist anymore, it was renamed to fg.

13–22 ↗(On Diff #10458)

Can get rid of some ul/li-specific styling

tomek requested changes to this revision.Mar 17 2022, 9:24 AM

We're using NavigationPanel in two places, so please also update SettingsSwitcher

web/sidebar/app-switcher.react.js
61 ↗(On Diff #10458)

Can we use something more semantic than div? Maybe button or a can work in this case?

This revision now requires changes to proceed.Mar 17 2022, 9:24 AM

rebase before addressing feedback

please also update SettingsSwitcher

will update this diff

Can we use something more semantic than div? Maybe button or a can work in this case?

sure, will give those a try

partially address feedback

tomek added inline comments.
web/sidebar/settings-switcher.react.js
7 ↗(On Diff #10501)

It's strange that we're using css from left-layout-aside. We should instead move this style to navigation-panel.css which is a lot better place for navigationPanelTab class

This revision is now accepted and ready to land.Mar 18 2022, 2:31 AM
web/sidebar/settings-switcher.react.js
7 ↗(On Diff #10501)

Yeah agree, I'll do this in a separate diff

This revision was landed with ongoing or failed builds.Mar 20 2022, 3:52 PM
This revision was automatically updated to reflect the committed changes.