-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fixing JS framework formatting for redirect pages #14914
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,12 +39,33 @@ export default async function Page(props: { | |
|
||
// get rid of irrelevant platforms for the `next` path | ||
const platformList = extractPlatforms(rootNode).filter(platform_ => { | ||
const node = nodeForPath(rootNode, [ | ||
// First check the main platform path | ||
let node = nodeForPath(rootNode, [ | ||
'platforms', | ||
platform_.key, | ||
...pathname.split('/').filter(Boolean), | ||
]); | ||
|
||
// If not found, check if it's a guide (like dart/guides/flutter) | ||
if (!node && platform_.guides) { | ||
for (const guide of platform_.guides) { | ||
node = nodeForPath(rootNode, [ | ||
'platforms', | ||
platform_.key, | ||
'guides', | ||
guide.name, | ||
...pathname.split('/').filter(Boolean), | ||
]); | ||
if (node) { | ||
// Create a copy of the platform with the guide URL to avoid mutating the original | ||
const platformCopy = {...platform_, url: guide.url}; | ||
// Update the platform reference to use the copy | ||
Object.assign(platform_, platformCopy); | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Platform URL Mutation Causes Framework Inheritance IssuesThe code mutates the original Additional Locations (1) |
||
} | ||
} | ||
} | ||
|
||
// extract title and description for displaying it on page | ||
if (node && title === defaultTitle && pathname.length > 0) { | ||
title = node.frontmatter.title ?? title; | ||
|
@@ -54,18 +75,78 @@ export default async function Page(props: { | |
return !!node; | ||
}); | ||
|
||
if (platformList.length === 0) { | ||
// For JavaScript platforms, also include individual frameworks that support the content | ||
const expandedPlatformList = [...platformList]; | ||
|
||
// Find JavaScript platform and add its supported frameworks | ||
// Only use JavaScript platform if it's already in the filtered list (has relevant content) | ||
const javascriptPlatform = platformList.find(p => p.key === 'javascript'); | ||
|
||
if ( | ||
javascriptPlatform && | ||
(pathname.startsWith('/session-replay/') || | ||
pathname.startsWith('/tracing/') || | ||
pathname.startsWith('/profiling/') || | ||
pathname.startsWith('/logs/')) | ||
) { | ||
// Get the JavaScript page to check which frameworks are supported | ||
const jsPageNode = nodeForPath(rootNode, [ | ||
'platforms', | ||
'javascript', | ||
...pathname.split('/').filter(Boolean), | ||
]); | ||
|
||
if (jsPageNode && jsPageNode.frontmatter.notSupported) { | ||
const notSupported = jsPageNode.frontmatter.notSupported; | ||
|
||
// Remove JavaScript from the main list temporarily | ||
const otherPlatforms = expandedPlatformList.filter(p => p.key !== 'javascript'); | ||
|
||
// Add supported JavaScript frameworks as separate entries | ||
const jsFrameworks: typeof platformList = []; | ||
javascriptPlatform.guides?.forEach(guide => { | ||
const guideKey = `javascript.${guide.name}`; | ||
if (!notSupported.includes(guideKey)) { | ||
jsFrameworks.push({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. feels like a lot of repetition : ) |
||
key: guideKey, | ||
name: guide.name, | ||
type: 'platform' as const, | ||
url: javascriptPlatform.url, | ||
title: guide.title, | ||
caseStyle: guide.caseStyle, | ||
sdk: guide.sdk, | ||
fallbackPlatform: guide.fallbackPlatform, | ||
language: guide.language, | ||
categories: guide.categories, | ||
keywords: guide.keywords, | ||
guides: [], | ||
integrations: [], | ||
icon: `javascript-${guide.name}`, | ||
}); | ||
} | ||
}); | ||
|
||
// Rebuild the list with JavaScript and its frameworks at the end | ||
expandedPlatformList.length = 0; // Clear the array | ||
expandedPlatformList.push(...otherPlatforms); // Add other platforms first | ||
expandedPlatformList.push(javascriptPlatform); // Add JavaScript platform | ||
expandedPlatformList.push(...jsFrameworks); // Add JavaScript frameworks last | ||
} | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if (expandedPlatformList.length === 0) { | ||
// try to redirect the user to the page directly, might result in 404 | ||
return redirect(next); | ||
} | ||
|
||
const requestedPlatform = Array.isArray(platform) ? platform[0] : platform; | ||
if (requestedPlatform) { | ||
const isValidPlatform = platformList.some( | ||
const validPlatform = expandedPlatformList.find( | ||
p => p.key === requestedPlatform?.toLowerCase() | ||
); | ||
if (isValidPlatform) { | ||
return redirect(`/platforms/${requestedPlatform}${pathname}`); | ||
if (validPlatform) { | ||
// Use the platform's URL (which may have been updated to point to a guide) | ||
return redirect(`${validPlatform.url}${pathname}`); | ||
} | ||
} | ||
|
||
|
@@ -82,19 +163,24 @@ export default async function Page(props: { | |
<Alert>{platformInfo}</Alert> | ||
|
||
<ul> | ||
{platformList.map(p => ( | ||
<li key={p.key}> | ||
<SmartLink to={`/platforms/${p.key}${pathname}`}> | ||
<PlatformIcon | ||
size={16} | ||
platform={p.icon ?? p.key} | ||
style={{marginRight: '0.5rem'}} | ||
format="sm" | ||
/> | ||
<h4 style={{display: 'inline-block'}}>{p.title}</h4> | ||
</SmartLink> | ||
</li> | ||
))} | ||
{expandedPlatformList.map(p => { | ||
// Check if this is a JavaScript framework (has javascript. prefix) | ||
const isJSFramework = p.key.startsWith('javascript.'); | ||
|
||
return ( | ||
<li key={p.key} style={{marginLeft: isJSFramework ? '20px' : '0'}}> | ||
<SmartLink to={`${p.url}${pathname}`}> | ||
<PlatformIcon | ||
size={16} | ||
platform={p.icon ?? p.key} | ||
style={{marginRight: '0.5rem'}} | ||
format="sm" | ||
/> | ||
<h4 style={{display: 'inline-block'}}>{p.title}</h4> | ||
</SmartLink> | ||
</li> | ||
); | ||
})} | ||
</ul> | ||
</DocPage> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be a lot more freeing to switch the type of this list from "platforms" to "platforms or guides"
from there, I see two options:
preferably with some flag indicating that it should be nested,
which will also open the door for a more generic solution without the hardcoded JS logic
OR
Then adjust the rendering accordingly
I tend to prefer approach 2, much cleaner