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] Increase printWidth from 80 to 85 #23512

Merged
merged 4 commits into from Nov 22, 2020
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 14, 2020

For tool-based formatting to be effective it has to be consistent. Everything else brings back the dark ages of "before-prettier".

PR serves as a confidence check whether #23509 was safe. If this PR introduces overflow then #23509 introduced potential overflow. Better to catch it now then having to monitor every PR for potential changes to formatting in the systems demos.

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Nov 14, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 14, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 56bf712

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 14, 2020

Note that they might be some overflow if we apply it at scale, the core of the tradeoff is that for the system, it's fine, it's different from the other parts of the documentation. I was hesitating with pushing further in this direction:

diff --git a/prettier.config.js b/prettier.config.js
index c99927730c..5fcca1752e 100644
--- a/prettier.config.js
+++ b/prettier.config.js
@@ -18,10 +18,10 @@ module.exports = {
       },
     },
     {
-      files: ['docs/src/pages/system/**/*.{js,tsx,md}'],
+      files: ['docs/src/pages/system/basics/**/*.{js,tsx,md}'],
       options: {
         // allow more space for the system
-        printWidth: 90,
+        printWidth: 100,
       },
     },
   ],

It could be better: #23509 (comment).

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 15, 2020

Having slept on it, How about we increase the limit to 100 #23512 (comment)? I think that it's a sane default. It's the value that we use for our sources. On this page's demo, it doesn't really matter if some content is offscreen, it doesn't contain important information the developers need to be able to read. On the other hand. it helps showcase the potential of the system in a prettier configuration that is "fair" :). cc @mbrookes and @mnajdova for feedback:

before
Capture d’écran 2020-11-15 à 11 55 42

after
Capture d’écran 2020-11-15 à 11 57 42

I have rushed 😆 #23509, I forgot to run yarn prettier write.

@mbrookes
Copy link
Member

Before is more readable for me

@oliviertassinari
Copy link
Member

@mbrookes "more readably" as easier to understand the DOM structure, the styles applied, or both?

@mbrookes
Copy link
Member

Both

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 19, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 19, 2020

90 is too high to be used on all the pages. For instance, all the demos of https://deploy-preview-23512--material-ui.netlify.app/components/accordion/ have a scrollbar. I would suggest we double down on the proposal of #23512 (comment).

The tradeoff is really about optimizing for reducing vertical space, using the same value as we use in the codebase: 100, which doesn't harm the demo, as not meant to be fully read. Same reason why https://tailwindcss.com/ has a horizontal scrollbar in all the demos of the homepage, it works better for demo purposes (and of course, not for documentation ones, nobody wants to scroll vertically to find important information).

@mbrookes
Copy link
Member

Horizontally truncated demos is what makes this hard to read.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 20, 2020

It's not meant to be read :), I think that developers should be able to easily skip the part with the sx when reading their code. Most often, you only care about the style when you need to work on it, you can/should be able to ignore them and focus on the logic.

@mbrookes
Copy link
Member

mbrookes commented Nov 20, 2020

It's not meant to be read :)

Okay, well then it should be easy enough to skip rendering code demos when any line overflows. (Or if you genuinely mean that, skip displaying them at all...)

@oliviertassinari
Copy link
Member

@mbrookes What I mean by "not read" is that the code that overflows in the system demo doesn't matter. What I think a developer wants to evaluate is how is this syntax is going to fix in his workflow. The developer's intent isn't to go copy & paste the part of the demo of the system that he needs, in which case, having to scroll is painful, as you can't have a holistic view of the content. Both vertically & horizontally scroll are painful. I personally prefer 100 because it allows to more easily understand the DOM structure, what are the different elements rendered? (I don't care about the styles, I will once I need to customize then, first, I need to be able to navigate this whole piece of I code I didn't write. I think that it better fits into the habits developers have when using a short className="", your HTML content is vertically dense.

Anyway. Happy to keep the current status-quo if there isn't a clear way forward. What matters is that it works with the audience.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 20, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Nov 20, 2020

Example for overflowing demo: https://deploy-preview-23512--material-ui.netlify.app/components/breadcrumbs/

90 print-width is not safe for our documentation at the moment.

@mbrookes
Copy link
Member

@oliviertassinari I'll say it again: Horizontally truncated demos affect readability. You can't scan the code – the pattern is interrupted. It doesn't matter whether that content is important, since it affects the readability of that which is. Clear?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 20, 2020

@mbrookes Ok, it makes sense. I guess it depends on the individual, we each have different sensibilities.

I personally don't feel interrupted on the tailwind's demos, even if they are truncated. The opposite, actually, I appreciate that the whole class name isn't cluttering my understanding of the DOM structure.

@oliviertassinari
Copy link
Member

90 print-width is not safe for our documentation at the moment.

I have pushed a commit to try 85 print-width.

@oliviertassinari oliviertassinari merged commit 3ab4356 into mui:next Nov 22, 2020
@oliviertassinari oliviertassinari changed the title [docs] Format docs [docs] Increase printWidth from 80 to 85 Nov 22, 2020
@oliviertassinari
Copy link
Member

I have browsed a bunch of demos, 85 seems to work, we save >1,000 LOCs :)

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.

None yet

4 participants