Skip to content

Add adaptive lighting#14877

Closed
KTibow wants to merge 41 commits into
home-assistant:nextfrom
KTibow:patch-9
Closed

Add adaptive lighting#14877
KTibow wants to merge 41 commits into
home-assistant:nextfrom
KTibow:patch-9

Conversation

@KTibow
Copy link
Copy Markdown
Contributor

@KTibow KTibow commented Oct 4, 2020

Proposed change

This adds docs for the new adaptive lighting component.

Type of change

  • Spelling, grammar or other readability improvements (current branch).
  • Adjusted missing or incorrect information in the current documentation (current branch).
  • Added documentation for a new integration I'm adding to Home Assistant (next branch).
  • Added documentation for a new feature I'm adding to Home Assistant (next branch).
  • Removed stale or deprecated documentation.

Additional information

Checklist

  • This PR uses the correct branch, based on one of the following:
    • I made a change to the existing documentation and used the current branch.
    • I made a change that is related to an upcoming version of Home Assistant and used the next branch.
  • The documentation follows the Home Assistant documentation standards.

@probot-home-assistant probot-home-assistant Bot added has-parent This PR has a parent PR in another repo next This PR goes into the next branch Hacktoberfest An PR on this issue (or the PR itself) is eligible towards Hacktoberfest! labels Oct 4, 2020
@KTibow KTibow marked this pull request as ready for review October 4, 2020 16:42
@klaasnicolaas klaasnicolaas added the new-integration This PR adds documentation for a new Home Assistant integration label Oct 4, 2020
@KTibow
Copy link
Copy Markdown
Contributor Author

KTibow commented Oct 4, 2020

Should this be a featured integration, so people can find out about it more easily?

@fti7
Copy link
Copy Markdown

fti7 commented Oct 5, 2020

Whats the difference to the already existing one? :-) https://www.home-assistant.io/integrations/flux/

@KTibow
Copy link
Copy Markdown
Contributor Author

KTibow commented Oct 5, 2020

It has more advanced config, UI configuration, and is based on top of the most popular component on HACS with 231 (!) stars. So I guess it's better...
See https://www.reddit.com/r/homeassistant/comments/j09219/any_circadian_lighting_users_good_news_i_just/?utm_source=share&utm_medium=web2x&context=3

@klaasnicolaas klaasnicolaas mentioned this pull request Oct 9, 2020
5 tasks
@basnijholt
Copy link
Copy Markdown
Contributor

@KTibow, thanks so much for this work. I wanted to make some changes (but have no permission), so I branched of your work and opened #15245.

@KTibow
Copy link
Copy Markdown
Contributor Author

KTibow commented Oct 14, 2020

Thanks! You can always hit the suggest changes button under review tab:
image
Drag it down to select more lines:
image
Then press this button:
image
Edit as needed, and post the comment.

Comment thread source/_integrations/adaptive_lighting.markdown Outdated
Comment thread source/_integrations/adaptive_lighting.markdown Outdated
Comment thread source/_integrations/adaptive_lighting.markdown
KTibow and others added 3 commits October 21, 2020 16:01
Co-authored-by: Bas Nijholt <basnijholt@gmail.com>
Co-authored-by: Bas Nijholt <basnijholt@gmail.com>
Co-authored-by: Bas Nijholt <basnijholt@gmail.com>
Comment thread source/_integrations/adaptive_lighting.markdown
Co-authored-by: Bas Nijholt <basnijholt@gmail.com>
Comment thread source/_integrations/adaptive_lighting.markdown Outdated
Co-authored-by: Bas Nijholt <basnijholt@gmail.com>
Comment thread source/_integrations/adaptive_lighting.markdown Outdated
Co-authored-by: Bas Nijholt <basnijholt@gmail.com>
Comment thread source/_integrations/adaptive_lighting.markdown Outdated
Copy link
Copy Markdown
Contributor

@mountainsandcode mountainsandcode left a comment

Choose a reason for hiding this comment

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

Sunrise_time is mentioned twice, sunset_time is missing

Comment thread source/_integrations/adaptive_lighting.markdown Outdated
Copy link
Copy Markdown
Contributor

@mountainsandcode mountainsandcode left a comment

Choose a reason for hiding this comment

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

@KTibow I noticed these parameters are declared optional in services descriptions - so suggestions below. May make sense to check all of the other services, too, so that they are aligned.

| Service data attribute | Optional | Description |
|------------------------|----------|--------------------------------------------------------------------------------------------------------------------------------------|
| `entity_id` | no | The `entity_id` of the switch in which to (un)mark the light as being "manually controlled". |
| `lights` | no | A light (or list of lights) to apply the settings to. |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `lights` | no | A light (or list of lights) to apply the settings to. |
| `lights` | Yes | A light (or list of lights) to apply the settings to. Default: All lights in the switch. |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

caps... they matter

|------------------------|----------|--------------------------------------------------------------------------------------------------------------------------------------|
| `entity_id` | no | The `entity_id` of the switch in which to (un)mark the light as being "manually controlled". |
| `lights` | no | A light (or list of lights) to apply the settings to. |
| `manual_control` | no | Whether to mark (true) or unmark (false) the light as "manually controlled", when not specified it selects all lights in the switch. |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `manual_control` | no | Whether to mark (true) or unmark (false) the light as "manually controlled", when not specified it selects all lights in the switch. |
| `manual_control` | yes | Whether to mark (true) or unmark (false) the light as "manually controlled", when not specified it selects all lights in the switch. Default: true |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Couldn't we add another column for defaults?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also on your next one, drag the plus button to suggest changes to multiple lines at once.

Co-authored-by: mountainsandcode <mountainsandcode@icloud.com>
Comment thread source/_integrations/adaptive_lighting.markdown Outdated
Comment thread source/_integrations/adaptive_lighting.markdown Outdated
KTibow and others added 2 commits December 19, 2020 07:31
Co-authored-by: Bas Nijholt <basnijholt@gmail.com>
Co-authored-by: Bas Nijholt <basnijholt@gmail.com>
Comment thread source/_integrations/adaptive_lighting.markdown
Co-authored-by: Bas Nijholt <basnijholt@gmail.com>
@klaasnicolaas
Copy link
Copy Markdown
Member

Damn what a lot of changes 😅

@KTibow
Copy link
Copy Markdown
Contributor Author

KTibow commented Dec 20, 2020

In terms of commits, or line numbers, on the docs PR or on the code PR? Yes, it's big.

@frenck
Copy link
Copy Markdown
Member

frenck commented May 28, 2021

Closing PR as the parent PR has been closed.

@frenck frenck closed this May 28, 2021
@probot-home-assistant probot-home-assistant Bot added Invalid and removed Hacktoberfest An PR on this issue (or the PR itself) is eligible towards Hacktoberfest! labels May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest-accepted has-parent This PR has a parent PR in another repo Invalid new-integration This PR adds documentation for a new Home Assistant integration next This PR goes into the next branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants