material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard
verified publisher icon mui

[docs] Add CodeSandbox/Stackblitz to the rest of the templates

Open siriwatknp opened this issue 1 year ago • 27 comments

Preview:

Follow up PR from #43604

  • Mostly deleting duplicated theme files in each template (switch to use the shared theme) 🫣 image
  • Fixed minor CSS after switching to shared TemplateFrame
  • Refactor theme.palette.* to use theme.vars.palette.*

siriwatknp avatar Sep 11 '24 10:09 siriwatknp

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad) No bundle size changes

Generated by :no_entry_sign: dangerJS against d75ead46ef488ee3f1c5c959fcd62e1beec1fab4

mui-bot avatar Sep 11 '24 10:09 mui-bot

@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 TemplateFrame:

  • There's an issue on the mobile layout of the Marketing-page, Sign-in, and Checkout templates: Screenshot 2024-09-11 at 08 55 53Screenshot 2024-09-11 at 08 56 13Screenshot 2024-09-11 at 08 58 59Screenshot 2024-09-11 at 08 57 54

  • The AppBar on Blog template is not centralized: Screenshot 2024-09-11 at 09 01 26

  • The drawer in the Blog/Marketing page template is placed behind the TemplateFrame: Screenshot 2024-09-11 at 09 00 37

  • The same issue happens in the mobile drawer of Checkout template: Screenshot 2024-09-11 at 08 58 16Screenshot 2024-09-11 at 08 58 10

  • There's a vertical scroll in the Checkout template:

https://github.com/user-attachments/assets/671b2405-3d39-4a4a-85ed-3875cdefdea9

  • There's an arrow icon behind the color mode select (same as in this comment): Screenshot 2024-09-11 at 08 58 59

zanivan avatar Sep 11 '24 12:09 zanivan

Updates:

  1. Make the theme selector smaller in mobile viewport
image
  1. Addressed all of the comments

siriwatknp avatar Sep 12 '24 03:09 siriwatknp

Question

Mostly deleting duplicated theme files in each template (switch to use the shared theme)

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 found

1. Checkout mobile dark mode Screenshot 2024-09-12 at 09 45 13

2. Blog images not showing up Screenshot 2024-09-12 at 09 44 06

3. Stackblitx only: Sessions graph width not working as expected

https://github.com/user-attachments/assets/5aef097d-ca1a-4037-9b85-3c47ec28d5da

DiegoAndai avatar Sep 12 '24 12:09 DiegoAndai

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

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

zanivan avatar Sep 12 '24 13:09 zanivan

Some bugs/issues I noticed while reviewing it again:

  • The TemplateFrame toolbar sometimes gets stuck when scrolling the dashboard template: Screenshot 2024-09-12 at 10 41 10

  • The AppBar from Blog and Marketing-page templates is not centralized: Screenshot 2024-09-12 at 10 44 22

  • There's an issue in all templates in both StackBlitz and CodeSandbox regarding the dark/light theme: Screenshot 2024-09-12 at 10 42 02 Screenshot 2024-09-12 at 10 43 52 Screenshot 2024-09-12 at 10 44 49 Screenshot 2024-09-12 at 10 44 59

Just a disclaimer, there are other small UI improvements since we changed the way the TemplateFrame works, and some other adjustments, that can be addressed in a future PR.

zanivan avatar Sep 12 '24 13:09 zanivan

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: https://github.com/mui/material-ui/pull/43220

DiegoAndai avatar Sep 12 '24 14:09 DiegoAndai

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).

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

zanivan avatar Sep 12 '24 16:09 zanivan

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

Thanks for pointing out. Let me clarify the removal of the duplicate theme. Using the shared-theme import has more benefits in several ways:

  • Better DX, updating the shared theme immediately update all the templates, no longer need to run the script every time you change the code. Also, when new template is added, you don't need to remember to update the script.
  • Reduced complexity, no script, no CI check. Faster feedback loop, if you forget to run the script, you need to wait for more 15-20 minutes until the CIs are green.
  • Almost 20k lines are removed, the repo size is already huge (> 120MB). If we could optimize it, we should do it.
  • The duplicated theme is not required to make the CodeSandbox/Stackblitz works.

With all the above reasons, I don't see why we should keep the duplicated theme.

siriwatknp avatar Sep 13 '24 02:09 siriwatknp

  • There's an issue in all templates in both StackBlitz and CodeSandbox regarding the dark/light theme:

This is a regression from https://github.com/mui/material-ui/pull/43632 due to <StyledEngineProvider injectFirst>. I'm investigating.

siriwatknp avatar Sep 13 '24 03:09 siriwatknp

  • The AppBar from Blog and Marketing-page templates is not centralized:

Can you explain what not centralized?

siriwatknp avatar Sep 13 '24 03:09 siriwatknp

@zanivan @DiegoAndai The dark mode bug that you see will be fixed by https://github.com/mui/material-ui/pull/43739.

siriwatknp avatar Sep 13 '24 05:09 siriwatknp

Can you explain what not centralized?

I don't know exactly if it's the AppBar or if it's the content, but there's a difference on the left/right alignment:

Screenshot 2024-09-13 at 09 26 27

zanivan avatar Sep 13 '24 12:09 zanivan

With all the above reasons, I don't see why we should keep the duplicated theme.

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:

  1. Update/remove the usage instructions on each template folder: https://github.com/mui/material-ui/tree/v6.1.0/docs/data/material/getting-started/templates/dashboard#usage, as not all files will be available in the templates' folders.
  2. Remove https://github.com/mui/material-ui/blob/master/docs/scripts/updateTemplatesTheme.ts
  3. Remove https://github.com/mui/material-ui/blob/master/package.json#L42

DiegoAndai avatar Sep 13 '24 17:09 DiegoAndai

  1. Update/remove the usage instructions on each template folder

Good point, did not know they exists.

Update all the readme to include copying shared-theme and remove the script. Waiting for https://github.com/mui/material-ui/pull/43739 to be released.

siriwatknp avatar Sep 16 '24 03:09 siriwatknp

@zanivan I could not see the difference. Is it the dashboard template?

image

siriwatknp avatar Sep 16 '24 03:09 siriwatknp

@zanivan I could not see the difference. Is it the dashboard template?

@siriwatknp It's happening on Marketing-page and Blog, and I found the reason: It's because my scroll bar is set to always visible, and this ends up causing the layout shift

zanivan avatar Sep 18 '24 17:09 zanivan

@zanivan I could not see the difference. Is it the dashboard template?

@siriwatknp It's happening on Marketing-page and Blog, and I found the reason: It's because my scroll bar is set to always visible, and this ends up causing the layout shift

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.

siriwatknp avatar Sep 20 '24 03:09 siriwatknp

Bringing @oliviertassinari 's feedback here:

Cool banner, but it looks like we need to continue to iterate on this:

  1. Doesn't work on mobile:

https://github.com/user-attachments/assets/bad5b1de-4507-4087-a280-eabacd859d83

  1. Can't really click on it:

https://github.com/user-attachments/assets/4cb59203-cd70-4a8b-aa21-7ed62a1420a5

zanivan avatar Sep 26 '24 07:09 zanivan

Bringing @oliviertassinari 's feedback here:

fixed.

siriwatknp avatar Oct 08 '24 11:10 siriwatknp

There's a bug with Dashboard background color:

Found the root cause, will open a separate PR but it's not related to this PR.

There's a bug with the theme Select where an icon appears behind the light/dark color mode toggle:

I could not see it in Chrome (in any templates), what's your browser and which template?

In almost all templates, when in mobile breakpoint, we end up losing the color mode toggle. Since users will likely see this in a split view, we need to have it there somehow—maybe we could draw some inspiration from the previous layouts that we had before adding the TemplateFrame, and bring it to CodeSandbox/Stackblitz UIs

Added the toggle to Dashboard, Marketing, and Blog mobile viewport (only show when open with Sandboxes)

Images aren't loading, probably because we use static images from the docs. Do you have any suggestions about how we might solve this?

I fixed it by using a new process.env.TEMPLATE_IMAGE_URL. In dev, it will use the / path but when deployed it will use the deployed url.

Sign-in, Sign-up and Sign-in-side color mode toggle could be improved, there are some bugs when clicking on it, as well as layout shift and overlapping.

Improved, can you check it again. For "there are some bugs when clicking on it", I think it comes from the custom styles from the theme, can you try to fix it?

siriwatknp avatar Oct 10 '24 06:10 siriwatknp

@siriwatknp I tried to update the branch from upstream to get the changes from the light/dark background color PR #44059 to review the PR again, but noticed that doing this it also committed 170a9c9. Is this right?

zanivan avatar Oct 10 '24 10:10 zanivan

I could not see it in Chrome (in any templates), what's your browser and which template?

It's happening on Sign-in, Sign-up and Sign-in-side. I've tested on Arc (chrome), Firefox and Safari, and it happens in all of these browsers.

For "there are some bugs when clicking on it", I think it comes from the custom styles from the theme, can you try to fix it?

Will work on it 👍

zanivan avatar Oct 10 '24 10:10 zanivan

There's a bug with the theme Select where an icon appears behind the light/dark color mode toggle

@zanivan found the issue, it's fixed in the latest change.

siriwatknp avatar Oct 14 '24 09:10 siriwatknp

Awesome—I think we're almost there! I just have some small remarks:

  1. On the custom theme, by default, we're only using the icons rounded, so we need to replace these in the StackBlitz/CodeSandbox (let me know if you want me to make this one):
import DarkModeIcon from '@mui/icons-material/DarkModeOutlined';
import LightModeIcon from '@mui/icons-material/LightModeOutlined';
  1. I don't know if it's expected, so correct me if I'm wrong, but images aren't loading on both StackBlitz/CodeSandbox;
  2. On the checkout template, we could add a little extra space atop, so we wouldn't have this overlapping components: Screenshot 2024-10-14 at 08 42 21

zanivan avatar Oct 14 '24 12:10 zanivan

@zanivan All fixed. There is one caveat for the images, when you open with CodeSandbox, the image will use https://mui.com as a host.

siriwatknp avatar Oct 15 '24 01:10 siriwatknp

@zanivan lets remember to tweet about this when it releases 🙏🏼

DiegoAndai avatar Oct 17 '24 15:10 DiegoAndai