Add Fumadocs #111
Open
Add Fumadocs #111
+4,769
−231
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@pomber is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
It looks like on mobile we're missing the hamburger that opens the sidebar nav. Will need an alternative. On the POC, the issue is that when on mobile, there's no way to open sidebar. The icon on the solana-com nav is for the entire website. On the current solana-com its floating on the bottom left: On the default fumadocs layout the icon to open the sidebar is part of the nav, but that's overridden by the default solana-com navbar. 
|
|
Minor styling things: The font type and font size may need to be updated to better match solana-com. The breadcrumbs should stick below the navbar when scrolling.
There's extra spacing above the TOC: 
|
|
There is also currently a bunch of "webpack-internal" warnings when running locally |
catalinred
reviewed
Nov 19, 2024
There was a problem hiding this comment.
@pomber Thanks for working on this!
Overall, this is looking promising, and I’m looking forward to seeing locale implementation for the app router. Having an alternative app router layout is something I’m particularly excited about, as it will help us migrate pages step-by-step from the pages router to the app router.
I second what John wrote above: first and foremost, we don’t want to lose any of the existing functionality. The UI should also match the current level of implementation, including hovers, active states, sizing, and overall typography.
Additionally, tables, alerts, and code snippets from within the /docs* are critical elements, and we need to ensure they are migrated properly.
P.S. Please fetch the latest changes from the main branch and apply them to the newly created next.config.mjs and rewrites-redirects.mjs.
| "@types/node": "^22.7.4", | ||
| "@types/styled-components": "^5.1.34", | ||
| "autoprefixer": "^10.4.20", |
There was a problem hiding this comment.
Is this really needed? Trying to understand how autoprefixer helps here since most modern browsers implemented standardized CSS properties with minimal need for prefixes.
There was a problem hiding this comment.
it's probably safe to remove, I need to check if Tailwind is using any newish feature that depends on prefixes though, because they recommend it in their docs
| @@ -73,17 +78,19 @@ | |||
| "postcss-preset-env": "^10.0.5", | |||
| "prettier": "^3.3.3", | |||
| "svg-react-loader": "^0.4.6", | |||
| "tailwindcss": "^3.4.14", | |||
There was a problem hiding this comment.
I read your comment about Tailwind and yeah am not a big fan of including tailwindcss once again and w/o the tw prefix this time. But we can accept that given the situation - looking forward though to have this polished as much as possible to reduce the amount of unused CSS here.
Curious if you have any other insights overall here, considering the solana-lib repo et all.
There was a problem hiding this comment.
I have a few questions about solana-lib, I'll ask later on slack
postcss.config.js
Outdated
There was a problem hiding this comment.
I read the postcss notes, and yeah these changes need to be reverted back, otherwise as you expect lots of stuff will break.
There was a problem hiding this comment.
yep I'll add the purge plugin back, it's a matter of writing the right regexp so it includes the missing tailwind classes, but I want to profile and compare the before and after output to make sure it's done right
I think the idea for most of those components is to use the ones built-in in fumadocs. For example, the steps component. We shouldn't lose any functionality, the current issues with the layout and styles are mainly because of the "minimum viable fumadocs integration" state of the PR. |
What are the errors? these? I think those were there from before, I can take a look if you want. I'm guessing they come from |
You're right it seems the warnings already existed. I just cloned the repo again, ran locally, and got the same warnings. Please leave them for now and prioritize the migration, thanks. |
|
Progress regarding CSS:
|
|
progress so far:
still missing:
|
|
thanks for the update, the migrated page ( the navigation for pages under docs/rpc doesn't seem to work clicking between pages. is the local routing issue a solveable problem? @catalinred, could you provide your feedback on the solutions regarding CSS and thoughts on the locale routing issue |
Yes, there's a similar use case that works here: https://github.com/amannn/next-intl/tree/main/examples/example-app-router-migration I'm going to try that, but with That solution does require adding a [lang] folder to the pages router and moving all existing pages there, though. It shouldn't cause issues, but it will mean extra testing to ensure that nothing breaks. |
yeah, I'm not yet sure what's happening there (it only happens on Vercel, not locally) |
|
@pomber fwiw, let me know if upgrading to next 15 would help in this case regarding the locale routing. For vis there's this branch I want to merge for a while already #71, but I'm waiting for an upcoming |
I think Next 15 still has the same issue with the I’ve found a middleware that does exactly what we need: https://nimpl.tech/docs/router. I’ve tested it, and it works well with both the Pages Router and the App Router. I still need to refine the implementation before committing the changes. These are changes to the pages router, so you can let me know if you spot any potential problems:
|
|
This feels like a bit of a stretch to me - all these to support both pages and app router. What do you think about going the other way around and transition everything to app router instead? Wouldn't this be more straightforward in this case? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a draft PR showing how to use fumadocs as the docs framework.
/docspath (with placeholder content)TODO
@ts-ignoreannotations that can be removed after the old docs framework is replaced (because of some conflicts between different versions ofmdx/types)RE Tailwind duplication:
fumadocs components ship with unprefixed classes, while solana-lib use the
twprefix, this means that we need to include two tailwind stylesheets (only for the routes that use fumadocs), we can still use something likecssnanoto remove any duplicated CSS (like tailwind's preflight styles)