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.

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

Fix: Selected framework in Code component #12496

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gkettani
Copy link

☕️ Reasoning

The Code component currently uses the Tabs component from the Nextra UI library to display different code examples for various frameworks. The Tabs component achieves synchronization across all instances by storing the selected tab's index in local storage. However, relying on an index introduces limitations when the tabs differ in content or order between components.

This PR refactors the Code component to handle tab synchronization and local storage management directly, decoupling this logic from the Tabs component. Instead of storing an index, the name of the framework is stored in local storage. This implementation:

  1. Stores the framework name in local storage instead of the index.
  2. Checks whether the stored framework name is valid for the current context.
  3. Displays the valid framework's tab or defaults to the first tab if no valid value is found.
  4. Synchronizes tabs across components using a StorageEvent, ensuring updates are propagated immediately.

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

Fixes: #12451

📌 Resources

@gkettani gkettani requested review from 0ubbe and ndom91 as code owners January 12, 2025 12:50
Copy link

vercel bot commented Jan 12, 2025

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

Name Status Preview Comments Updated (UTC)
auth-docs ❌ Failed (Inspect) Jan 16, 2025 7:49pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
next-auth-docs ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2025 7:49pm

Copy link

vercel bot commented Jan 12, 2025

@gkettani is attempting to deploy a commit to the authjs Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@halvaradop halvaradop left a comment

Choose a reason for hiding this comment

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

Great work! Just a few changes needed

const [selectedFramework, setSelectedFramework] = useState<string>(
Object.values(renderedFrameworks)[0]
)

const updateFrameworkStorage = (value: string): void => {
const params = new URLSearchParams(searchParams?.toString())
params.set("framework", value)
Copy link
Contributor

@halvaradop halvaradop Jan 16, 2025

Choose a reason for hiding this comment

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

To avoid repeating the use of parseParams throughout the code, please move it to the updateFrameworkStorage function.

Comment on lines 90 to +104
useEffect(() => {
const length = Object.keys(renderedFrameworks).length
function handleStorage(event: StorageEvent) {
if (
event.key === AUTHJS_TAB_KEY &&
event.newValue &&
Object.values(renderedFrameworks).includes(event.newValue)
) {
setSelectedFramework(event.newValue)
}
}
window.addEventListener("storage", handleStorage)
return () => {
window.removeEventListener("storage", handleStorage)
}
}, [])
Copy link
Contributor

@halvaradop halvaradop Jan 16, 2025

Choose a reason for hiding this comment

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

I think this logic is unnecessary because changing the framework in the tabs will automatically update the selected framework, regardless of the active tab or whether you have instances of the page open in different windows on your computer. It will synchronize the updates.

Copy link
Author

Choose a reason for hiding this comment

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

You're correct; that was indeed the case when we used the storageKey prop in the Tabs component. The Tabs component managed the synchronization logic across all tabs.

<Tabs
storageKey={AUTHJS_TAB_KEY}
items={Object.values(renderedFrameworks)}
>

However, because the Tabs component relies on an index to track and sync the selected tab, this approach caused the issue we’re addressing.
To resolve this, I extracted the synchronization logic from the Tabs component and adapted it to our use case. During this process, I reviewed the implementation of the Tabs component to understand its behavior and made targeted adjustments to ensure it works properly in our context, specifically by replacing the index-based tracking with framework names.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your explanation, that makes sense. When you remove the storageKey prop from Tabs, it becomes necessary to configure how the selected value is updated. For this, you’ve used the selectedIndex prop, which allows changing the selected value. However, by not using the storageKey prop, you’ve synchronized the selected value between the tabs of the page using the storage event to address the issue. Only the currently active tab updates the value, while the others remain unchanged. This is a great approach to solving the problem!

if (!getFrameworkStorage) {
window.localStorage.setItem(AUTHJS_TAB_KEY, "0")
handleFrameworkChange(Object.values(renderedFrameworks)[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simply use the value selectedFramework

Comment on lines 85 to 87
if (textContent) {
handleFrameworkChange(textContent)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if the selected value is within renderedFrameworks, as selecting a tab in the package manager will update the framework value.

Copy link
Contributor

@halvaradop halvaradop left a comment

Choose a reason for hiding this comment

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

Great work! We just need to wait for the maintainer to merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selected framework is not consistent across multiple Code components in the page.
2 participants