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

[docs] Codesandbox has missing files #9474

Closed
MmtBkn opened this issue Dec 12, 2017 · 16 comments
Closed

[docs] Codesandbox has missing files #9474

MmtBkn opened this issue Dec 12, 2017 · 16 comments
Assignees
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.
Milestone

Comments

@MmtBkn
Copy link

MmtBkn commented Dec 12, 2017

Hello,

It's great to have code sandbox in docs. It'll save me a lot of time while prototyping.

I realized that, drawers(completely) and components with images are not working properly.

Avatar;

Drawer;
https://material-ui.com/demos/drawers/
https://codesandbox.io/s/zpro6nj7m

Gridlist;
https://codesandbox.io/s/r4mk39vkxq

etc.

Thank you for your effort.

@MmtBkn MmtBkn changed the title [docs] Codesanbox missing files [docs] Codesandbox has missing files Dec 12, 2017
@lukePeavey
Copy link
Contributor

@mbrookes @oliviertassinari Is ok if I do some work on this issue?

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 12, 2017

We use an import to avoid code duplication between the demos. It was done on purpose. I don't think that the issue should change the tradeoff. The best option I can think of is to disable the codesandbox link for those demos.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Dec 12, 2017
@lukePeavey
Copy link
Contributor

@oliviertassinari Sorry, I should have been more clear. I wasn't talking about modifying the demos. I thought there would be some way to include external files when creating the sandbox, so i meant that i would work on that.

@lukePeavey
Copy link
Contributor

@oliviertassinari Would something like this be possibility?
Commit 3d7ffaa

If not, i can take care of removing the sandbox links on these pages for now.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 14, 2017

@lukePeavey Thanks for giving a shot at the issue and exploring it! I still think that the simplest option is to disable the codesandbox like by adding some explicit params in the .md. I don't think that having codesandbox working everywhere worth the extra complexity.

@oliviertassinari oliviertassinari added this to the Future milestone Dec 14, 2017
@lukePeavey
Copy link
Contributor

Ok, that makes sense.

@mbrookes
Copy link
Member

mbrookes commented Dec 14, 2017

Add Button to the list, due the use of the internal Link component.

Now fixed.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 16, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 17, 2017

Ok, #9521 is setting up the infrastructure needed for hiding the codesandox button on the conflicting demos.

@lukePeavey
Copy link
Contributor

Ok, #9521 is setting up the infrastructure needed for hiding the codesandox button on the conflicting demos.

Could i take care of adding this option to the rest of demos (Drawer + GridList)?

@oliviertassinari
Copy link
Member

@lukePeavey It would be awesome. I'm not sure which demos need it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 18, 2017

@lukePeavey Raised an interesting point. We have some weird HTTP 414 in:

  • Stepper (HorizontalNonLinearAlternativeLabel)
  • Popover (AnchorPlayground)
  • Table (EnhancedTabled)

@oliviertassinari
Copy link
Member

I don't understand. This PR #9491 was lost along the way. We need to apply it back! Then we can close this issue 🎉 .

@lukePeavey
Copy link
Contributor

One more thing before you close this...

This might be a separate issue, but /static/ images also don't work in code sandbox. This affects a number of demos including Chips, Avatar, Button, Card.

@lukePeavey
Copy link
Contributor

Would it work to replace relative urls to /static/ with absolute urls in the sandbox content?
https://github.com/mui-org/material-ui/blob/627ffda5b565b6a6be99d3c429438e4a2c559ffd/docs/src/modules/components/Demo.js#L101

@oliviertassinari
Copy link
Member

Would it work to replace relative urls to /static/ with absolute urls in the sandbox content?

Oh, it's something you added in a previous PR. Yeah. It's a simple fix :).

@lukePeavey
Copy link
Contributor

Ok, cool

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Dec 25, 2017
@oliviertassinari oliviertassinari self-assigned this Dec 27, 2017
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this issue Dec 27, 2017
We have done quite some iteration on this issue.
We should be in a good shape now.
I think that it would be good in the future to add a link
toward a different online editor.

@lukePeavey I haven't ported your image replacement logic.
I couldn't find it. Feel free to submit a pull-request
to add it in the future :).

Closes mui#9474
oliviertassinari added a commit that referenced this issue Dec 27, 2017
We have done quite some iteration on this issue.
We should be in a good shape now.
I think that it would be good in the future to add a link
toward a different online editor.

@lukePeavey I haven't ported your image replacement logic.
I couldn't find it. Feel free to submit a pull-request
to add it in the future :).

Closes #9474
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

No branches or pull requests

4 participants