-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
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
Configuration.h LCD & SDCard section rewrite #3496
Conversation
// Panucatt VIKI LCD with status LEDs, integrated click & L/R/U/P buttons, separate encoder inputs | ||
// | ||
// | ||
// Activate this directive if you have a Panucatt VIKI LCD with status LEDs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Activate this directive" is maybe too geek-show-offy. How about "Enable this option"?
Overall I like the changes. Better documentation never hurts. |
bc2cf77
to
20534f3
Compare
@thinkyhead I've updated, please check if the texts are aligned to your feedback. |
// | ||
//#define SPI_SPEED SPI_HALF_SPEED | ||
//#define SPI_SPEED SPI_QUARTER_SPEED | ||
//#define SPI_SPEED SPI_EIGHTH_SPEED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things like this are going to mess with my configurator, which expects to find only a single line defining an option (unless overriding with some condition later). It could accommodate "placeholder" items like these, but it's a little odd semantically. I kind of think this goes too far in trying to be "helpful" so users only need to uncomment something – no typing required. But, these are the options we have… we have to expose them somehow… so I will leave this in place for now.
For the most part all the new documentation is very clear, much better than the old brief comments. I've made a commit with various changes, mostly to make the language more concise, remove usage of the words "you" and "your", add blank lines between options, adopt a more "passive" or "neutral" voice, and so on… |
20534f3
to
37f716e
Compare
Thanks for the feedback, I incorporated your patch. |
👍 Looks great for me. |
I still have to replicate this to all the config files once we settle the main one. |
I had a lot of extra time, so I multiplied these changes across the other configs. I can submit that patch shortly. |
I've updated the
Configuration.h
comment section for LCD Panels and SDcard options. I've not updated all configuration files because I would like first to gather feedback about this new layout.Followup #3484, #3493