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

Make it possible to use type "area" for area cards instead of Mushroom template card #52

Merged
merged 11 commits into from
Oct 13, 2023

Conversation

punxaphil
Copy link
Contributor

@punxaphil punxaphil commented Sep 18, 2023

What

This provides an option to use type: area instead of mushroom-template-card for Areas, by configuring use_ha_area_card: true.
Along with that an option is also added to set common options for all area cards. Example:

strategy:
  type: custom:mushroom-strategy
  options:
    areas:
      _:
        use_ha_area_card: true

Of course it was already possible to override type in mushroom-strategy, but that wouldn't work with the type: area, since it also requires area: <area id>.

Why

  • The built in area card in Home Assistant supports pictures, which means that the user of mushroom-strategy now does not have to provide picture URLs for every area.
  • The built in area-card shows pictures on the entire card, making it easier to see what is on the image (compared to the mushroom template card which only shows the picture as a round icon).

Example

Example of how it can look:
image

@punxaphil
Copy link
Contributor Author

punxaphil commented Sep 19, 2023

@AalianKhan @ajagnanan @DigiLive would love to hear your input on this 😄

@ajagnanan
Copy link

ajagnanan commented Sep 19, 2023

I think it's a good addition! Is there a way to move the section header Areas to align in the right column with the areas? That's happening on my dashboards as well.

Edit: I see it's fixed with #50

@DigiLive
Copy link
Collaborator

DigiLive commented Sep 19, 2023

@johanfrick Thank you for your effort in enhancing the strategy.

I do like the looks, but I have some objections also...

  1. It's called Mushroom Strategy, designed to auto-populate the dashboard with Mushroom cards, whenever useful and possible.
    I'm not sure we should mix all kind of card-styles together, but to keep it at a minimum instead.

  2. I.m.o., If adding support for the HA Area Card, the way to go is to add a new class to the package.
    The sole purpose of the current AreaCard class is to create a Mushroom-styled area card.

  3. Adding properties to the configuration is allowed, so the following example should work.
    (However, additional properties may be incompatible with Mushroom Strategy.)

    areas:
      biljardrum:
        type: area
        area: biljardrum
        navigation_path: biljardrum

My advice to @AalianKhan is to enable the discussions in the settings of this repository and talk about this request and/or poll the user's opinions in a dedicated topic.

@punxaphil
Copy link
Contributor Author

Yeah, you make a fair point that this is a strategy for Mushroom cards. However, this is also the greatest strategy I've seen for Home Assistant and I think the crowd can be made a lot wider if we allow some more customisation. Many people prefer Mushroom cards and nothing else, but not everyone does.
That being said, I'll give it another think how I can "hide" the intention of not using Mushroom card for some parts. Probably by moving more to the user's configuration of mushroom-strategy.
But let me know if it is not worth the effort, and I'll move forward by only maintaining my own fork.

Repository owner locked and limited conversation to collaborators Sep 20, 2023
@DigiLive
Copy link
Collaborator

Locked to continue the discussion at #58

README.md Outdated Show resolved Hide resolved
src/cards/AreaCard.js Outdated Show resolved Hide resolved
src/views/HomeView.js Outdated Show resolved Hide resolved
src/views/HomeView.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Repository owner unlocked this conversation Sep 24, 2023
@DigiLive
Copy link
Collaborator

I've unlocked this PR so the commits can be discussed.
Please discuss the more generic things in the discussion: #58

@punxaphil
Copy link
Contributor Author

Please review again @DigiLive . I have updated the code according to your comments. I have also updated the README, and the PR description.

src/typedefs.js Outdated Show resolved Hide resolved
src/cards/HaAreaCard.js Outdated Show resolved Hide resolved
@punxaphil
Copy link
Contributor Author

@DigiLive Ready for another review. I have updated according to your comments.

@DigiLive DigiLive self-assigned this Sep 29, 2023
@DigiLive DigiLive added the enhancement New feature or request label Sep 29, 2023
@DigiLive
Copy link
Collaborator

Please don't push any other commits as I'm reviewing and tweaking locally now.

I really want to avoid merge conflicts at this point.

@DigiLive
Copy link
Collaborator

DigiLive commented Oct 1, 2023

@johanfrick Can you evaluate my code changes, please?

An error would be shown in the console (in debug mode) if there's no
type set for any of the cards. This is fixed by giving the type a
default value.
@punxaphil
Copy link
Contributor Author

@DigiLive I have evaluated the code. It looks reasonable to me. I have also verified it by trying it out locally. I am happy to go forward with this solution. 👍

@punxaphil
Copy link
Contributor Author

Anything else needed before merging?

@DigiLive DigiLive dismissed their stale review October 1, 2023 10:29

Reviewed while making changes.

@DigiLive DigiLive requested a review from AalianKhan October 1, 2023 10:29
@DigiLive DigiLive merged commit 98ef9fe into AalianKhan:main Oct 13, 2023
@punxaphil punxaphil deleted the set_type_area branch October 13, 2023 06:20
@maslyankov
Copy link

This looks well if the rooms have no temp/humidity sensors. If there are sensors for an area, they show up in the area card and therefore the UI looks too narrow and bad. The easiest solution would be to make them appear in one column.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants