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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use icon image where available #4721

Merged
merged 13 commits into from Feb 4, 2020
Merged

Use icon image where available #4721

merged 13 commits into from Feb 4, 2020

Conversation

ludeeus
Copy link
Member

@ludeeus ludeeus commented Feb 1, 2020

Proposed change

From: https://twitter.com/andreadonno/status/1218074534936563712?s=20

  • Use icon image where available in both the dashboard and store
  • If add-on is installed and stopped or not available on the system, use grayscale on the image.
  • If add-on is installed and there is an update pending, show a dot in the corner.

I tried to add a dot in the bottom corner of the icon to indicate the started/stopped statuses, but the images come in every shape possible and it did not look that great, using grayscale always works.

Baloob mentioned and "Update pening" bage on twitter, but that had some issues with jumping text, and when the title was long it should drop the title/go over multiple lines, I think a little dot in the corner is sufficient.

Screenshots 馃摲

Overview

image

Add-on store

image

Add-on not startet/not available

image

Add-on has an update pending

image

"old" overview (for refrence)

image

"old" add-on store (for refrence)

image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!) maybe?
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@balloob
Copy link
Member

balloob commented Feb 2, 2020

How do we know if something is installed in the store ?

@@ -17,21 +18,44 @@ class HassioCardContent extends LitElement {
@property() public hass!: HomeAssistant;
@property() public title!: string;
@property() public description?: string;
@property({ type: Boolean }) public available?: boolean;
@property({ type: Boolean }) public available: boolean = true;
@property({ type: Boolean }) public updateAvailable: boolean = false;
Copy link
Member

@balloob balloob Feb 2, 2020

Choose a reason for hiding this comment

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

Since this is a generic card (personally not a fan, too many different permutations we're trying to support in 1 card), all properties to this card should be named after how they impact the look of the card and not what they represent in the specific use case that caused them to be added.

Copy link
Member Author

@ludeeus ludeeus Feb 2, 2020

Choose a reason for hiding this comment

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

How about creating a base card, and a add-on card that extends it?

Copy link
Member

Choose a reason for hiding this comment

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

<ha-card> 馃槅

@ludeeus
Copy link
Member Author

ludeeus commented Feb 2, 2020

How do we know if something is installed in the store ?

Good question, there is the obvious "it's your system, you should know what you have installed"
How about a - installed postfix in the title?

@balloob
Copy link
Member

balloob commented Feb 2, 2020

Maybe a blue line on the top of the card? (but with position absolute, not border, becaus we don't want to increase height of the card or mis-align the text)

@ludeeus
Copy link
Member Author

ludeeus commented Feb 2, 2020

Should that be limited to the store view only?

@balloob
Copy link
Member

balloob commented Feb 2, 2020

Yes

@ludeeus
Copy link
Member Author

ludeeus commented Feb 2, 2020

Updated store screenshot to reflect the addition of the blue bar on installed add-ons

.iconImage=${ha105pluss && addon.icon
? `/api/hassio/addons/${addon.slug}/icon`
: undefined}
?showBlueTopbar=${addon.installed}
Copy link
Member

Choose a reason for hiding this comment

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

It's always faster to go via properties. Only use attributes if the value never changes.

Suggested change
?showBlueTopbar=${addon.installed}
.showBlueTopbar=${addon.installed}

private _addonTapped(ev: any): void {
navigate(this, `/hassio/addon/${ev.currentTarget.addon.slug}`);
}

private _openStore(): void {
navigate(this, "/hassio/store");
}

private get _computeHA105plus(): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Personally not a fan of getters when doing computation. It's only 1 call here so it's ok.

? "hassio:arrow-up-bold-circle"
: "hassio:puzzle"}
.iconTitle=${addon.state !== "started"
? "Add-on is stopped"
Copy link
Member

Choose a reason for hiding this comment

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

Do we have i18n strings for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, it's on my list to create PR's (one pr section, to keep it small) to have that for all stings (that make sense) under hassio/src

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Pretty goood. Ok to merge when final comments addressed.

@bramkragten
Copy link
Member

Should we combine the update available and installed indication, I think it is weird that we have 2 different styles to indicate something. Why not have a orange line?

@balloob
Copy link
Member

balloob commented Feb 3, 2020

I think that's a great idea. We can use secondary color if an update is available.

@bramkragten bramkragten merged commit f1a1654 into home-assistant:dev Feb 4, 2020
@bramkragten bramkragten added the Supervisor Related to the supervisor panel label Feb 4, 2020
@lock lock bot locked and limited conversation to collaborators Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed Supervisor Related to the supervisor panel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants