Page MenuHomePhabricator

[web] Fix tabs in modals displaying incorrectly
ClosedPublic

Authored by inka on Feb 20 2023, 6:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 4, 1:11 PM
Unknown Object (File)
Mar 6 2024, 1:36 AM
Unknown Object (File)
Feb 27 2024, 10:21 PM
Unknown Object (File)
Feb 27 2024, 10:21 PM
Unknown Object (File)
Feb 27 2024, 10:21 PM
Unknown Object (File)
Feb 27 2024, 10:21 PM
Unknown Object (File)
Feb 27 2024, 10:21 PM
Unknown Object (File)
Feb 27 2024, 10:21 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-3049/fix-tabs-header-container
In D6734 I missed the fact that Tabs was also used in some modals. This made them display incorrectly - not like in our designs. This diff fixes that by applying different styles based on a
flag passed as a prop. I dodn't create a separate component, because they would differ in only two lines. Instead, I change the css they use, like we do in NavigationPanel.

Before fix:

image.png (268×740 px, 16 KB)

image.png (390×816 px, 24 KB)

After fix:

image.png (262×738 px, 20 KB)

image.png (412×792 px, 26 KB)

And the tabs in chat still styled according to the new designs:
image.png (352×738 px, 31 KB)

Here are the tabs on commit d6865be7150cb1e2c63bfd64faf03c34af842163, before my changes that introduces the bug (they look the same):

image.png (252×746 px, 20 KB)

image.png (420×808 px, 30 KB)

Test Plan

Run we app, checked that tabs in modals display as they used to, and tabs in Chat display according to the new designs

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka edited the summary of this revision. (Show Details)
inka edited the summary of this revision. (Show Details)

Fix label colour

inka added inline comments.
web/components/tabs-underline.css
1–35 ↗(On Diff #22727)

This is the old version of the css used in tabs. I just brought it back

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 20 2023, 9:04 AM
Harbormaster failed remote builds in B16620: Diff 22727!
inka requested review of this revision.Feb 20 2023, 10:14 AM

This is an urgent task, so would be great if somebody could give it a review

If you need, feel free to add me to review

PS – please rebase on master to fix iOS build

Looks good, but please do rebase on master.

This revision is now accepted and ready to land.Feb 21 2023, 6:56 AM
tomek requested changes to this revision.Feb 21 2023, 7:04 AM
tomek added inline comments.
web/components/tabs-header.js
15 ↗(On Diff #22853)

This name is misleading. How would a caller know what's the result of specifying isPillStyle={false}. What is being styled as a pill?

A better approach would be to introduce a prop and specify what is being style. It seems like headerType or headerStyle might be a good idea with possible values "pill" | "underline".

This revision now requires changes to proceed.Feb 21 2023, 7:04 AM
web/components/tabs-header.js
15 ↗(On Diff #22853)

Ohh, that's so much better, I'll do that, thank you

Change +isPillStyle?: boolean, to +headerStyle?: 'pill' | 'underline',

tomek added inline comments.
web/components/tabs-header.js
20–23 ↗(On Diff #22866)

You don't have to memoize this. The css value is one constant or another, so the memoization isn't beneficial.

web/components/tabs.react.js
13 ↗(On Diff #22866)

Can we avoid repeating this type by extracting it somewhere?

This revision is now accepted and ready to land.Feb 21 2023, 8:31 AM