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

Configuration.h LCD & SDCard section rewrite #3496

Closed

Conversation

jbrazio
Copy link
Contributor

@jbrazio jbrazio commented Apr 14, 2016

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

// 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,
Copy link
Member

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"?

@thinkyhead
Copy link
Member

Overall I like the changes. Better documentation never hurts.

@jbrazio
Copy link
Contributor Author

jbrazio commented Apr 16, 2016

@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
Copy link
Member

@thinkyhead thinkyhead Apr 16, 2016

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.

@thinkyhead
Copy link
Member

thinkyhead commented Apr 16, 2016

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…

2884c9f

@jbrazio jbrazio force-pushed the doc/sdcard-panel-rewrite branch from 20534f3 to 37f716e Compare April 16, 2016 13:48
@jbrazio
Copy link
Contributor Author

jbrazio commented Apr 16, 2016

Thanks for the feedback, I incorporated your patch.

@CONSULitAS
Copy link
Contributor

👍 Looks great for me.
@thinkyhead Should we merge and multiply? For K8200 we need a new release soon, so i can do on my own...

@jbrazio
Copy link
Contributor Author

jbrazio commented Apr 16, 2016

I still have to replicate this to all the config files once we settle the main one.

@thinkyhead
Copy link
Member

I had a lot of extra time, so I multiplied these changes across the other configs. I can submit that patch shortly.

@thinkyhead thinkyhead closed this Apr 17, 2016
@jbrazio jbrazio deleted the doc/sdcard-panel-rewrite branch April 17, 2016 03:46
@jbrazio jbrazio modified the milestone: 1.1.0 Jul 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants