bokub / home-assistant-extension

πŸ“Œ Home Assistant browser extension
MIT License
171 stars 3 forks source link

Extract Popup to Vue, add multiple dashboards support, minor corrections #9

Closed Chriserus closed 3 years ago

Chriserus commented 3 years ago

Description

My name is Krzysztof and I am a Home Assistant enthusiast like you. I came across your extension and I think it's great! Easy to use, light, yet very useful. I really wanted to see multiple dashboards support, so here I am, with my PR :)

This PR includes changes made in order to make it possible to have multiple dashboards linked to the extension. I extracted Popup as a new Vue component and used it in main view, but as well in the preview view (in my opinion it makes developing further changes a whole lot easier). When it comes to add-ons - I had no previous experience with them, as I only develop web applications on a daily basis, so feel free to tell me all your remarks to my code, or maybe some tips for the future :) As an important note: I had a hard time styling components and I am not proud of how the buttons look like now (better see for yourself), but there is an idea for the future to make it possible for the user to choose buttons colors (dashboard ones).

When it comes to testing - I run the extension both in Chrome and in Firefox.

I will leave some comments discussing my motivation for certain changes.

I hope you'll find my PR well, I am waiting for your feedback.

Chriserus commented 3 years ago

Hi,

Thanks a lot for your PR πŸ‘

I like the idea of using a Vue component for the Popup, it makes the code a little bit cleaner and that's cool.

However, I don't see the point of creating multiple dashboards and having these tabs, which (let's be honest) are not really pretty, when you could simply create a dedicated Lovelace dashboard and use the default Home Assistant header to have tabs ! It looks like you created a feature that already existed for years in Home Assistant

You also introduced a few style regressions (some shadows have disappeared, centered content is not centered anymore, some margins have been removed...)

Even if I don't think I'll merge this, I made some comments in case you want to improve your own version

Thanks anyway and have a great day

The thing is, tabs from Home Assistant (at least in my case) cannot be changed inside the iFrame, that's why I made it this way. It has to do something with CORS. Try it out yourself, maybe I don't see something and you're right. In my case, when I don't hide header and can see it -> I cannot change the tabs at all, I can only be inside one of them.

When it comes to style - true, as I mentioned, that was the hard part for me :)

bokub commented 3 years ago

tabs from Home Assistant (at least in my case) cannot be changed inside the iFrame

That is very strange.. Do you have this issue in both Chrome and Firefox?

Do you have any error message in your JS console?

Chriserus commented 3 years ago

tabs from Home Assistant (at least in my case) cannot be changed inside the iFrame

That is very strange.. Do you have this issue in both Chrome and Firefox?

Do you have any error message in your JS console?

Chrome: Uncaught DOMException: Blocked a frame with origin MY_DUCKDNS_URL from accessing a cross-origin frame. Firefox: Uncaught DOMException: Permission denied to access property "history" on cross-origin object

I focused on a chrome error and found out that it's some security protection, that you can't change the page inside iFrame, maybe I missed something.

bokub commented 3 years ago

That is weird because I've never had any problem, either in chrome or Firefox, so it might be related to your setup

I don't know, but maybe you use caddy or some other web server that adds security headers, or maybe you have a different config from mine.. :man_shrugging:

Chriserus commented 3 years ago

Are you using https? I wonder if it's a thing, that some people aren't able to change tabs. I might later post an issue, maybe we'll find more people with the problem.

bokub commented 3 years ago

Yes, I'm using https:// with the default port, my URL looks like https://xxx.yyyy.com/lovelace/extension

Chriserus commented 3 years ago

To conclude, this error is maybe happening to some users, so the change won't be used. Yet I still see value in my solution, there are people that use multiple HA instances (for example - summer home). In my opinion, this change isn't that invasive and you would like to have the vue component extracted. I can try to fix styling, as you mentioned, I broke it with my changes and I'll style the buttons (change it to tabs probably). My question is, will you then consider merging this PR? If not, I will probably won't do those changes just for me, if yes, I promise to correct all of these things ;) I don't see a problem to integrate it into your project!

Have a nice day and let me know what you think! Cheers

bokub commented 3 years ago

Well, I do not intend to add the "multiple tabs" features, because in my opinion it's an edge case that brings complexity to the extension for no real added value:

However, if you make a PR with just the Popup.vue refactoring, I'll consider merging it, but of course, you won't see any difference on the final result.