-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
[docs-infra] Add CodeSandbox/Stackblitz to the rest of the templates #43708
base: master
Are you sure you want to change the base?
Conversation
@siriwatknp, these are the issues I found during my initial review. I won't dive too deep into visual tweaks for the templates, like the box-shadow on the marketing page appbar, since that's not the focus of this PR. I've kept the feedback primarily focused on layout and interaction issues based on
Screen.Recording.2024-09-11.at.08.57.18.mov
|
Question
I thought the idea was to have duplicate theme files so each template would have all the code. This way users can clone one template without having to navigate to the shared theme. Is this no longer the case? @zanivan Some issues found3. Stackblitx only: Sessions graph width not working as expected Screen.Recording.2024-09-12.at.09.42.37.mov |
This was the idea, as the only way users could get the templates before was by copying the folder from the project. However, integrating with CodeSandbox/StackBlitz solves this issue, since @siriwatknp packaged the shared theme within the template files when opened on either platform |
This makes sense. I'll leave it up to you to decide. I think we could be a bit more cautious: We have this new and improved way to use the templates with the sandboxes, and we should promote it, but it might not be best to "remove" the old way right away. There's no rush; we can wait and see how users react to this new system, and if it sticks, we can remove the old one (duplicated themes). If you decide to remove the duplicated files, then let's also remove the copying mechanism and CI check, which were added here: #43220 |
No strong opinion on this. Maybe we can wait until the feature to copy the theme to the clipboard is ready before removing the theme folders from the templates |
Thanks for pointing out. Let me clarify the removal of the duplicate theme. Using the shared-theme import has more benefits in several ways:
With all the above reasons, I don't see why we should keep the duplicated theme. |
This is a regression from #43632 due to |
Can you explain what not centralized? |
@zanivan @DiegoAndai The dark mode bug that you see will be fixed by #43739. |
I understand the reasons, and I fully agree that we should do it at some point 👍🏼 my question was, "Are we ready to remove the previous DX?". @zanivan and @siriwatknp agree that we should, so let's do it. Then my point becomes that we need to:
|
Good point, did not know they exists. Update all the readme to include copying |
@zanivan I could not see the difference. Is it the dashboard template? |
@siriwatknp It's happening on |
…late/new-structure
Got it. The way I see to improve it is to switch the header to position sticky but I think it should be done outside of this PR. |
Preview:
Follow up PR from #43604
🫣
theme.palette.*
to usetheme.vars.palette.*