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] New code blocks #764

Merged
merged 15 commits into from
Oct 30, 2024
Merged

[docs] New code blocks #764

merged 15 commits into from
Oct 30, 2024

Conversation

vladmoroz
Copy link
Contributor

@vladmoroz vladmoroz commented Oct 28, 2024

@vladmoroz vladmoroz added the docs Improvements or additions to the documentation label Oct 28, 2024
@vladmoroz vladmoroz requested a review from colmtuite October 28, 2024 18:28
@mui-bot
Copy link

mui-bot commented Oct 28, 2024

Netlify deploy preview

https://deploy-preview-764--base-ui.netlify.app/

Generated by 🚫 dangerJS against 7b4a2d1

Comment on lines +6 to +7
import 'docs/src/styles.css';
import 'docs/src/styles/style.css';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is temporary just to support the current setup in place, 'docs/src/styles/style.css' will go away eventually

@vladmoroz
Copy link
Contributor Author

vladmoroz commented Oct 28, 2024

@michaldudak do you have a clue why docs in paths breaks deploys? 5c2549d

I was having minor problems with path resolution that were fixed by adding docs path to tsconfig, but Netlify builds break then

@michaldudak
Copy link
Member

tsconfig fields aren't merged with parent config settings but replaced, so by adding compilerOptions.paths, you overwrote the paths defined in /tsconfig.base.json. Try adding the path to the base config instead.

@michaldudak
Copy link
Member

The demos on old pages are getting messed up a bit. Would it be problematic to actually edit the styles on the existing pages instead of the /new ones?


:root {
--color-content: #fff;
--font-mono: 'Menlo', 'Consolas', monospace;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the default font OK for Linux users?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll add a couple more fallbacks

@@ -127,9 +126,9 @@ async function loadSimpleDemo(path: string, variantName: string): Promise<DemoVa
const jsFilePath = mainFilePath.replace(/\.tsx?$/, '.js');
if (mainFileLanguage === 'ts' && existsSync(jsFilePath)) {
const jsContent = await readFile(jsFilePath, 'utf-8');
const jsPrettyPromise = await codeToHtml(jsContent, {
const jsPrettyPromise = highlighter.codeToHtml(jsContent, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const jsPrettyPromise = highlighter.codeToHtml(jsContent, {
const jsPrettyContent = highlighter.codeToHtml(jsContent, {

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 29, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 29, 2024
@vladmoroz
Copy link
Contributor Author

vladmoroz commented Oct 29, 2024

The demos on old pages are getting messed up a bit. Would it be problematic to actually edit the styles on the existing pages instead of the /new ones?

Oh man this annoys me the hell out of me, but I don't know if it's worth our time to refactor styles on the old pages. The root issue is that .Content ... selector that targets all the elements on the page indiscriminately and messes up styles of anything you put on the page. I think we'll have to live with it for a bit, because otherwise I just need to refactor a thing we won't use soon at all

Anything particular that's messed up and bothers you?

@vladmoroz
Copy link
Contributor Author

vladmoroz commented Oct 29, 2024

@michaldudak looks like unrelated tests are failing after rebasing, I assume I can ignore them?

@michaldudak
Copy link
Member

I think we'll have to live with it for a bit, because otherwise I just need to refactor a thing we won't use soon at all

Anything particular that's messed up and bothers you?

It's nothing blocking. The main issue is that demos render in dark mode (following my prefers-color-scheme), while the rest of the page is light, and they have some extra horizontal scrollbars.


Aside from this, I just realized it could be nice to detect cmd/ctrl + a when a demo source is focused and select just the demo source instead of the whole page (could be done in a separate PR, of course)

@michaldudak
Copy link
Member

michaldudak commented Oct 29, 2024

As for the broken tests - this looks like an issue after migrating to vitest. I'll see what's going on right away.

EDIT: likely fixed in #765

@vladmoroz
Copy link
Contributor Author

Aside from this, I just realized it could be nice to detect cmd/ctrl + a when a demo source is focused and select just the demo source instead of the whole page (could be done in a separate PR, of course)

Agree, added that

As for the broken tests - this looks like an issue after migrating to vitest. I'll see what's going on right away.

EDIT: likely fixed in #765

Thanks!

@vladmoroz vladmoroz merged commit 8e8afd0 into mui:master Oct 30, 2024
18 checks passed
@vladmoroz vladmoroz deleted the code-blocks branch October 30, 2024 15:43
@michaldudak
Copy link
Member

One thing I'm missing is the Reset button. It used to come in handy to reload the demo to its initial state.

@vladmoroz
Copy link
Contributor Author

One thing I'm missing is the Reset button. It used to come in handy to reload the demo to its initial state.

We decided to remove it, among other things, since there was too much going on in the toolbar. I agree it has its uses, but it's probably more so for us as we maintain and test the components in the docs way more often than the end user would.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants