-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
add collapse and tab examples to guides/contributing.rst #4359
Conversation
✅ Deploy Preview for esphome ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe pull request introduces substantial updates to the contributing guidelines for the ESPHome project. It includes new sections on collapsible content and tabs in reStructuredText (RST), along with refined guidelines for submitting pull requests. The changes aim to enhance the documentation's structure and clarity, providing contributors with clearer instructions and examples. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
guides/contributing.rst (1)
269-278
: Update example text for consistency.The example text in the rendered collapsible sections doesn't match the text in the code block above. Let's update it for consistency:
.. collapse:: Open :open: - This section is expected to be open by default. + This section is open by default.This change will make the example more consistent with the code block shown earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- guides/contributing.rst (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
guides/contributing.rst (1)
Pattern
**
: - Do not generate or add any sequence diagrams
🪛 GitHub Check: build
guides/contributing.rst
[failure] 258-258:
Directive 'collapse' is not followed by an empty line. Please insert an empty line after guides/contributing.rst:258
[failure] 262-262:
Directive 'collapse' is not followed by an empty line. Please insert an empty line after guides/contributing.rst:262
[failure] 265-265:
Directive 'collapse' is not followed by an empty line. Please insert an empty line after guides/contributing.rst:265
[failure] 266-266:
Directive 'code-block' is not followed by an empty line. Please insert an empty line after guides/contributing.rst:266
[failure] 281-281:
Directive 'code-block' is not followed by an empty line. Please insert an empty line after guides/contributing.rst:281
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- guides/contributing.rst (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
guides/contributing.rst (1)
Pattern
**
: - Do not generate or add any sequence diagrams
🪛 GitHub Check: build
guides/contributing.rst
[failure] 269-269:
Directive 'collapse' is not followed by an empty line. Please insert an empty line after guides/contributing.rst:269
[failure] 273-273:
Directive 'collapse' is not followed by an empty line. Please insert an empty line after guides/contributing.rst:273
🔇 Additional comments (1)
guides/contributing.rst (1)
254-300
: Overall approval with suggested improvementsThe addition of collapsible sections to the contributing guide is a valuable improvement. It provides clear examples and enhances the documentation's flexibility. With the suggested formatting fixes and additional context, this section will be even more helpful for contributors.
Please implement the following improvements:
- Fix the formatting issues by adding required blank lines.
- Add context about when to use collapsible sections.
- Enhance the note about required blank lines after
collapse
directives.Once these changes are made, this addition will be an excellent enhancement to the contributing guidelines.
🧰 Tools
🪛 GitHub Check: build
[failure] 269-269:
Directive 'collapse' is not followed by an empty line. Please insert an empty line after guides/contributing.rst:269
[failure] 273-273:
Directive 'collapse' is not followed by an empty line. Please insert an empty line after guides/contributing.rst:273
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
guides/contributing.rst (1)
254-302
: LGTM! Consider adding more context about collapsible sections.The new content about collapsible sections is clear and provides useful examples. However, it would be beneficial to add a brief explanation of when and why to use collapsible sections in the documentation, as suggested in a previous review comment. This could include use cases and best practices for ESPHome documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- guides/contributing.rst (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
guides/contributing.rst (1)
Pattern
**
: - Do not generate or add any sequence diagrams
🔇 Additional comments (1)
guides/contributing.rst (1)
254-302
: Formatting issues resolved.The previously mentioned formatting issues, such as missing blank lines after directives, have been addressed in this version of the code. The content now follows the correct formatting guidelines.
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
guides/contributing.rst (1)
254-302
: Improve consistency and clarity in the collapsible sections exampleThe new content about collapsible sections is a valuable addition. However, there are a few minor improvements we can make:
- Ensure consistent indentation in the code examples. The content inside the
collapse
directives should be indented with 4 spaces for better readability.- Consider making the note about required blank lines more prominent, perhaps by using a warning directive instead.
Here's a suggested improvement for the note:
.. warning:: A blank line is *required* after every ``collapse`` directive. This ensures proper parsing of the reStructuredText and prevents rendering issues in the final documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- guides/contributing.rst (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
guides/contributing.rst (1)
Pattern
**
: - Do not generate or add any sequence diagrams
🪛 GitHub Check: build
guides/contributing.rst
[failure] 314-314:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:314
[failure] 318-318:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:318
[failure] 322-322:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:322
[failure] 330-330:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:330
[failure] 334-334:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:334
[failure] 338-338:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:338
[failure] 351-351:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:351
[failure] 355-355:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:355
[failure] 359-359:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:359
[failure] 363-363:
Directive 'tab' must be indented with 4 spaces, not 2. See guides/contributing.rst:363
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.
Couple little nitpicks, otherwise lgtm
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Oh, you changed a bunch of stuff...haha...will re-review... |
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.
Take 2 😄
Co-authored-by: Keith Burzinski <kbx81x@gmail.com>
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
guides/contributing.rst (4)
256-286
: Improve code block indentation for consistency.The indentation in the code blocks for the collapsible section examples is inconsistent. Please adjust the indentation to make it uniform across all examples. This will improve readability and make it easier for users to understand the correct syntax.
Here's an example of how the indentation should look:
.. code-block:: rst .. collapse:: Details Something small enough to escape casual notice. .. collapse:: A long code block .. code-block:: yaml # Sample configuration entry switch: - platform: gpio name: "Relay #42" pin: GPIOXX
302-305
: Clarify the note about the:open:
flag.The note about the
:open:
flag and the required blank line could be more explicit to avoid confusion.Consider rephrasing the note as follows:
.. note:: - The `:open:` flag must be placed on the line immediately following the `collapse` directive, without a blank line in between. - A blank line is *required* after the content of every `collapse` directive.This clarification will help users understand exactly where to place the
:open:
flag and where the blank line is required.
310-340
: Improve code block indentation and expand explanation.The indentation in the code blocks for the tabs examples is inconsistent. Additionally, the explanation of when to use tabs could be expanded to provide more context for users.
Please adjust the indentation in all code blocks to be consistent. Use 4 spaces for each level of indentation.
Consider expanding the introduction to tabs with more context. For example:
- **Tabs**: Use the `tabs` directive to group related content that users might want to switch between. This is particularly useful for: - Showing configuration examples for different platforms or versions - Presenting alternative methods to achieve the same result - Organizing complex information into more manageable sections The `tabs` directive defines a tab set, and individual tabs are added using the `tab` directive:This additional context will help users understand when and why they might want to use tabs in their documentation.
406-412
: Enhance the note about tabs usage.The note at the end of the tabs section could be more prominent and detailed to provide better guidance to users.
Consider replacing the current note with a more comprehensive one:
.. important:: When using tabs: - A blank line is *required* after every `tabs` and `tab` directive. - The contents of each tab can be displayed by clicking on the tab label. - Clicking again on the currently open tab will hide its content, showing only the tab labels. - Avoid nesting tabs too deeply, as it can make the content hard to navigate. - For advanced features like tab groupings, refer to the `sphinx-tabs documentation <https://sphinx-tabs.readthedocs.io/en/latest/>`_.This enhanced note provides more detailed guidance on using tabs effectively and points users to additional resources for advanced usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- guides/contributing.rst (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
guides/contributing.rst (1)
Pattern
**
: - Do not generate or add any sequence diagrams
🔇 Additional comments (1)
guides/contributing.rst (1)
254-412
: Excellent additions to the contributing guideThe new sections on collapsible content and tabs are valuable additions to the ESPHome contributing guide. These features will allow contributors to create more structured and interactive documentation, improving the overall user experience.
A few minor suggestions have been made to improve consistency and clarity:
- Adjusting indentation in code blocks for better readability.
- Clarifying the usage of the
:open:
flag in collapsible sections.- Expanding the explanation of when to use tabs and enhancing the usage notes.
Overall, these additions significantly enhance the documentation capabilities for ESPHome contributors. Great work on implementing these new features!
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.
This works for me and visually this looks fine on both Chrome & Safari on macOS and in Safari in iOS.
Description:
To file guides/contributing.rst ,
examples have been added regarding how to use collapsible sections and tabs.
Source:
https://sphinx-toolbox.readthedocs.io/en/latest/extensions/collapse.html
https://sphinx-tabs.readthedocs.io/en/latest/
Related issue (if applicable): fixes
None
Pull request in esphome with YAML changes (if applicable): esphome/esphome#
None
Checklist:
I am merging into
next
because this is new documentation that has a matching pull-request in esphome as linked above.or
I am merging into
current
because this is a fix, change and/or adjustment in the current documentation and is not for a new component or feature.Link added in
/index.rst
when creating new documents for new components or cookbook.