Add nina integration #56647
Add nina integration #56647
Conversation
|
The remaining errors are generated by the black git hook. I have tried to fix them, but the changes are always reverted. |
There was a problem hiding this comment.
Thank you for your contribution!
I have added some review comments.
Also, here are some more general thoughts:
- This is not really user friendly, can we change this somehow?
- The config flow is really slow and not very responsive, selecting something causes the browser to freeze for at least 3 seconds until anything happens.
- You are creating entities based on user input, you should take care of the edge cases.
Feel free to tag me again once you have implemented the feedback accordingly or if you have any follow up questions.
|
@marvin-w I have now split the city selection into 6 dropdown menus. So there is only a short lag when opening. Would this be a suitable option? |
|
I don't see any new commits, but it looks better |
|
@DeerMaximum Please make sure you are following our developer guidelines in our developer documentation. https://developers.home-assistant.io/ Install the pre-commit hook locally in order to avoid formatting issues or make sure to run those checks manually before pushing. |
|
Left some comments, please fix the cla-error |
There was a problem hiding this comment.
Nice! Thanks for the fast integration!
After retesting I found out that if the Corona Filter is active it will still "block" the space for those entities and move the relevant warnings to the entity under it.
I'd have expected 1 and 2 to be used instead of 3 and 4 - can you please have a look again?
So 1 and 2 should be Unsafe and not 3 and 4. Can we change this?
Co-authored-by: Marvin Wichmann <marvin@fam-wichmann.de>
There was a problem hiding this comment.
Looks good to me, thanks @DeerMaximum!
Tested locally and working nicely. Code looks good. Documentation is available. Brands PR is created.
I've asked in discord if one more pair of eyes can give it a quick look.
|
Thanks @DeerMaximum! |
| { | ||
| **{ | ||
| vol.Optional(region): cv.multi_select(self.regions[region]) | ||
| for region in CONST_REGIONS |
There was a problem hiding this comment.
This is a weird form. Why are we splitting it in different region groups? Why not just have one field for all regions?
There was a problem hiding this comment.
If this was done to solve browser performance, it seems we're working around an issue instead of solving it.
There was a problem hiding this comment.
If I remember correctly the reason was browser performance: #56647 (review)
Should I open an issue for it instead? There are a lot of possible inputs here and it will definitely lag if this is done in a single field.
There was a problem hiding this comment.
Yes, we should find out why there's lag.
Breaking change
Proposed change
This pull request added the support to retrieve warnings from the German NINA app.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: