Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rework tabs query state to work better across both routers #23253

Merged
merged 4 commits into from May 2, 2024
Merged

rework tabs query state to work better across both routers #23253

merged 4 commits into from May 2, 2024

Conversation

charislam
Copy link
Contributor

@charislam charislam commented Apr 25, 2024 •
edited

I have read the CONTRIBUTING.md file.

YES

What kind of change does this PR introduce?

Refactor + enhancement

What is the current behavior?

Tabs uses a messy combination of next/router and custom events to handle syncing state with other tabs + table of contents.

What is the new behavior?

Refactor tabs so that:

  • Behavior add-ons, such as query params, are HOCs in ui-patterns, to keep the basic Tabs minimal
  • No dependency on next/router (this is to smooth out a separate App Router migration PR)
  • No dependency on useSearchParams either (this is to preserve static generation of as much HTML content as possible, for SEO purposes -- see comment about client-rendering bailout when using useSearchParams)
  • Syncing behavior better localized in a hook

Minor enhancement to save tab preferences to local storage so they are remembered between sessions

Additional context

Add any other context or screenshots.

@supabase Supabase
Copy link

supabase bot commented Apr 25, 2024

No changes detected in supabase directory.
This pull request has been ignored for the connected project xguihxuzqibwxjnimxev due to its connection settings.
Go to Project Integrations Settings ↗︎ in order to change this behavior.

Branching Preview Branches by Supabase.
Learn more about Supabase for Git ↗︎.

@vercel Vercel
Copy link

vercel bot commented Apr 25, 2024 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
database-new ✅ Ready (Inspect) Visit Preview May 2, 2024 5:03pm
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2024 5:03pm
studio-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2024 5:03pm
zone-www-dot-com ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2024 5:03pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
studio ⬜️ Ignored (Inspect) Visit Preview May 2, 2024 5:03pm
studio-self-hosted ⬜️ Ignored (Inspect) Visit Preview May 2, 2024 5:03pm
ui-storybook ⬜️ Ignored (Inspect) May 2, 2024 5:03pm

MildTomato
MildTomato reviewed Apr 29, 2024
View reviewed changes
packages/ui/src/components/Tabs/Tabs.tsx
@@ -70,61 +67,31 @@ const Tabs: React.FC<PropsWithChildren<TabsProps>> & TabsSubComponents = ({
children?.[0]?.props?.id
)

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we maybe just make a UI pattern component instead?

Currently trying to move away from using this component to the /shadcn/ui/tabs one, but even so, the logic of queryGroup could be kept in some sort of wrapper component around that Tabs component, so the other Tabs are not burdened with it.

Wdyt?

Could have a quick huddle to spitball some ideas on that, not really thought it through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good point, this is trying to do much for a basic tab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, I've pulled it out as a HOC. Decided on a HOC because I have another PR that adds another functionality to tabs (stickiness) and I want to be able to compose the two, but open to other suggestions

@charislam charislam requested a review from a team as a code owner April 30, 2024 22:35
charislam
charislam commented Apr 30, 2024
View reviewed changes
apps/studio/components/interfaces/SQLEditor/DownloadSnippetModal.tsx
@@ -2,7 +2,7 @@ import type { ModalProps } from '@ui/components/Modal/Modal'
import { snakeCase } from 'lodash'
import Link from 'next/link'
import { useState } from 'react'
import { Button, CodeBlock, IconExternalLink, Modal, Tabs, TabsProvider } from 'ui'
Copy link
Contributor Author

@charislam charislam Apr 30, 2024 •
edited

Choose a reason for hiding this comment

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

TabsProvider was only necessary to save selection state if there were multiple sets of tabs within the context, and has been superseded by the withQueryParams HOC. This doesn't have multiple sets of tabs, so doesn't need either.

@charislam charislam merged commit 2f4f0ad into master May 2, 2024
16 checks passed
@charislam charislam deleted the refactor/tabs branch May 2, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MildTomato MildTomato MildTomato left review comments

@saltcod saltcod saltcod approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@charislam @saltcod @MildTomato