Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Convert tabbedview to functional component #12478

Merged
merged 6 commits into from
May 3, 2024
Merged

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented May 1, 2024

The 'Tab' is still a class, so now it's a functional component that has a supporting class, which is maybe a bit... jarring, but I think is actually perfectly logical.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

dbkr added 2 commits May 1, 2024 10:48
The 'Tab' is still a class, so now it's a functional component that
has a supporting class, which is maybe a bit... jarring, but I think
is actually perfectly logical.
@dbkr dbkr added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label May 1, 2024
@dbkr dbkr marked this pull request as ready for review May 1, 2024 10:47
@dbkr dbkr requested a review from a team as a code owner May 1, 2024 10:47
@dbkr dbkr requested review from florianduros and MidhunSureshR May 1, 2024 10:47
dbkr added a commit that referenced this pull request May 1, 2024
This does mean the logic of keeping what tab is active is now in each
container component, but for a functional component, this is a single
line. It makes TabbedView simpler and the container components always
know exactly what tab is being displayed rather than having to effectively
keep the state separately themselves if they wanted it.

Based on #12478
@dbkr dbkr mentioned this pull request May 1, 2024
4 tasks
Copy link
Contributor

@florianduros florianduros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I think we can improve it by creating more small components for its children and add more documentation

src/components/structures/TabbedView.tsx Outdated Show resolved Hide resolved
src/components/structures/TabbedView.tsx Show resolved Hide resolved
src/components/structures/TabbedView.tsx Show resolved Hide resolved
src/components/structures/TabbedView.tsx Outdated Show resolved Hide resolved
src/components/structures/TabbedView.tsx Outdated Show resolved Hide resolved
dbkr added 2 commits May 3, 2024 13:02
and use contitional call syntax
Copy link
Contributor

@florianduros florianduros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@dbkr dbkr added this pull request to the merge queue May 3, 2024
Merged via the queue into develop with commit 050f617 May 3, 2024
30 checks passed
@dbkr dbkr deleted the dbkr/tabbedview_func branch May 3, 2024 13:22
github-merge-queue bot pushed a commit that referenced this pull request May 3, 2024
* Convert tabbedview to functional component

The 'Tab' is still a class, so now it's a functional component that
has a supporting class, which is maybe a bit... jarring, but I think
is actually perfectly logical.

* put comment back

* Fix bad tab ID behaviour

* Make TabbedView a controlled component

This does mean the logic of keeping what tab is active is now in each
container component, but for a functional component, this is a single
line. It makes TabbedView simpler and the container components always
know exactly what tab is being displayed rather than having to effectively
keep the state separately themselves if they wanted it.

Based on #12478

* Fix some types & unused prop

* Remove weird behaviour of using first tab is active isn't valid

* Don't pass initialTabID here now it no longer has the prop

* Fix test

* bleh... id, not icon

* Change to sub-components

and use contitional call syntax

* Comments

* Fix element IDs

* Fix merge

* Test DesktopCapturerSourcePicker

to make sonarcloud the right colour

* Use custom hook for the fllback tab behaviour
github-merge-queue bot pushed a commit that referenced this pull request May 7, 2024
* Convert tabbedview to functional component

The 'Tab' is still a class, so now it's a functional component that
has a supporting class, which is maybe a bit... jarring, but I think
is actually perfectly logical.

* put comment back

* Fix bad tab ID behaviour

* Make TabbedView a controlled component

This does mean the logic of keeping what tab is active is now in each
container component, but for a functional component, this is a single
line. It makes TabbedView simpler and the container components always
know exactly what tab is being displayed rather than having to effectively
keep the state separately themselves if they wanted it.

Based on #12478

* Move the active tab in user settings to the dialog title

Separated by a colon, as per the new design.

* Update snapshots

* Update a playwright test

* Fix more tests / snapshots

* Attempt to test all the cases of titleForTabID

* More tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants