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

Cookbook entry for Display component #173

Merged
merged 13 commits into from
Feb 26, 2019
Merged

Cookbook entry for Display component #173

merged 13 commits into from
Feb 26, 2019

Conversation

ahd71
Copy link
Contributor

@ahd71 ahd71 commented Feb 25, 2019

I created a cookbook entry how to use the display component.
(First github pull request and first time using reStructuredText so if it isn't up to the mark, please let me know what to fix and I'll do that and send another pull request)

@ahd71
Copy link
Contributor Author

ahd71 commented Feb 25, 2019

Apperently I didn't get the links correct

10:46:31 AM: /opt/build/repo/cookbook/display.rst:14: WARNING: undefined label: /components/display/ssd1306_i2c (if the link has no caption the label must precede a section header)
10:46:31 AM: /opt/build/repo/cookbook/display.rst:123: WARNING: unknown document: /api/display_8h.html
10:46:31 AM: /opt/build/repo/cookbook/display.rst:131: WARNING: unknown document: /components/display
10:46:31 AM: /opt/build/repo/cookbook/display.rst:133: WARNING: unknown document: /api/display_8h.html

@ahd71
Copy link
Contributor Author

ahd71 commented Feb 25, 2019

Noticed the warning about tabs vs spaces, have fixed them :-)

Fixed some syntax errors in reStructuredText which is totally new for me.
@ahd71
Copy link
Contributor Author

ahd71 commented Feb 25, 2019

Hi,

Need help Otto :-)

Get these warnings (possible some more but I think I fixed them)

/opt/build/repo/cookbook/display.rst:29: WARNING: Content block expected for the "warning" directive; none found.

Don't know what is wrong

/opt/build/repo/cookbook/display.rst:119: WARNING: unknown document: https://esphome.io/api/display_8h.html

I understand it is wrong syntax but didn't know how to properly refer to another repo

File cookbook/display.rst contains tab character on line 47:0. Please convert tabs to spaces.

I have removed all tabs and replaced with 4 spaces but still throwing an error.

@ahd71
Copy link
Contributor Author

ahd71 commented Feb 25, 2019

I have another image to use in the table of content. Once this page works fine I'll update the toc page too.

Copy link
Member

@OttoWinter OttoWinter left a comment

Choose a reason for hiding this comment

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

Thanks! Some comments:

  • The name "display" for the cookbook entry is a bit too generic. I'd prefer something more specific like "Time & Temperature OLED Display"
  • The image needs to be resized a bit - even if it's not on the main page 2MB is quite big (and slows down every people downloading the docs to edit them locally)
  • In the opening paragraph, it would be useful to list what this config will do, it doesn't have to be long just a short list so that people don't need to read through the entire page (image is also good like it is already, but a bit of text too won't hurt)

@ahd71
Copy link
Contributor Author

ahd71 commented Feb 25, 2019

Great. Again, I'm new to github so I think I just reverted the updates you made. I'll fix that (as well as your review comments - including the size of the second file I just uploaded)

- Updated the title and renamed the file
- Resized the images
- highlighted that 'model' also could be different
- small cosmetic changes
Copy link
Member

@OttoWinter OttoWinter left a comment

Choose a reason for hiding this comment

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

👍 The guide itself looks good now. The two images could be renamed though to match the new cookbook entry name.

And you will have to add it to the index.rst file so that it is listed.

@ahd71
Copy link
Contributor Author

ahd71 commented Feb 26, 2019

Done the changes discussed. Possibly the title in the index (which matches the page title) is too long - I don't know how it will be rendered - shorten it if you like!

Thanks for all support with my first pull request!

@ahd71
Copy link
Contributor Author

ahd71 commented Feb 26, 2019

Looks like the title become two liner but looked okay in my view. The image was not displayed correctly though - is that due to the "rendering preview engine" or something that I should fix?

@ahd71
Copy link
Contributor Author

ahd71 commented Feb 26, 2019

yeay, finally! Everything looks okay now as far as I can see!

Copy link
Member

@OttoWinter OttoWinter left a comment

Choose a reason for hiding this comment

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

🎉

@OttoWinter
Copy link
Member

Looks like the title become two liner but looked okay in my view. The image was not displayed correctly though - is that due to the "rendering preview engine" or something that I should fix?

Not a problem in this case.

@OttoWinter OttoWinter merged commit 3c348b6 into esphome:current Feb 26, 2019
@esphome esphome locked and limited conversation to collaborators Jun 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants