-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Conversation
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). |
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: I have rushed 😆 #23509, I forgot to run |
Before is more readable for me |
@mbrookes "more readably" as easier to understand the DOM structure, the styles applied, or both? |
Both |
bc14c9a
to
89f8eac
Compare
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). |
Horizontally truncated demos is what makes this hard to read. |
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. |
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...) |
@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 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. |
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. |
@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? |
@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. |
I have pushed a commit to try 85 print-width. |
I have browsed a bunch of demos, 85 seems to work, we save >1,000 LOCs :) |
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.