Page MenuHomePhabricator

[web] Refactor `ChatTabs` to use `Tabs` component
ClosedPublic

Authored by jacek on Mar 9 2022, 3:20 AM.
Tags
None
Referenced Files
F3487491: D3373.id10186.diff
Wed, Dec 18, 8:08 AM
F3487448: D3373.id10303.diff
Wed, Dec 18, 7:49 AM
F3487443: D3373.id10301.diff
Wed, Dec 18, 7:46 AM
F3487429: D3373.id10941.diff
Wed, Dec 18, 7:43 AM
F3486398: D3373.diff
Wed, Dec 18, 4:50 AM
Unknown Object (File)
Wed, Nov 27, 4:28 AM
Unknown Object (File)
Nov 7 2024, 9:16 AM
Unknown Object (File)
Nov 5 2024, 9:41 PM

Details

Summary

Used introduced Tabs component in ChatTabs, replacing existing solution. Chat borders design now matches Figma design and elements have onHover effect. The behavior of panel should remain the same.

Before:

Screenshot_Google Chrome_2022-03-09_122729.png (152×834 px, 14 KB)

After:

Screenshot_Google Chrome_2022-03-09_122708.png (164×844 px, 14 KB)

Test Plan

Run web app and try to switch Between Focus and Background tabs. It should work as before.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Mar 10 2022, 2:50 AM

Just an idea: we can consider using children instead of a prop with an array. Initially I thought it might be complicated, but I used this approach in D3341 and it wasn't that hard

web/chat/chat-tabs.react.js
29–33 ↗(On Diff #10186)

Can we use more concrete type, e.g. 'Background' | 'Focus' and avoid using invariants?

66 ↗(On Diff #10186)
This revision now requires changes to proceed.Mar 10 2022, 2:50 AM

Used Tabs.Container and Tabs.Item

tomek added a reviewer: ashoat.
tomek added inline comments.
web/chat/chat-tabs.react.js
29 ↗(On Diff #10303)

Just to make sure, as it looks really interesting. So does that mean that if we try to use a tab with a different id a flow error would be returned?

web/chat/chat-tabs.react.js
29 ↗(On Diff #10303)

Yes, if we try to execute this callback with other id, flow error is thrown.

Makes sense to me! The Tabs.Item header prop probably could've been a Tabs.Header component, but it doesn't matter that much

This revision is now accepted and ready to land.Mar 14 2022, 9:21 PM