Skip to content
This repository was archived by the owner on Jan 13, 2026. It is now read-only.

Add plugin badge in the catalog + minor UI fixes for multiplugin#3749

Merged
antgamdia merged 8 commits into
vmware-tanzu:masterfrom
antgamdia:icons-2
Nov 15, 2021
Merged

Add plugin badge in the catalog + minor UI fixes for multiplugin#3749
antgamdia merged 8 commits into
vmware-tanzu:masterfrom
antgamdia:icons-2

Conversation

@antgamdia
Copy link
Copy Markdown
Contributor

@antgamdia antgamdia commented Nov 11, 2021 •
edited
Loading

Description of the change

Following up the #3747 (comment), this is a small PR implementing one of the proposals

Benefits

It'll be easier to identify which plugin a pkg belongs to?

Possible drawbacks

N/A

Applicable issues

Additional information

Before:
image

After:
image

image

@antgamdia
Add pkg badge in pkg view
34f6ece 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Merge branch 'master' into icons-2
7c7b1e6
@antgamdia
Run css linter
9706313 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
absoludity
absoludity approved these changes Nov 11, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

Nice. I'm slightly colourblind, so not the best person to comment on colours, but the dark-mode colour almost looks to my eyes like an alert.

Only other thought, as I mentioned on our other chat, I wonder if capitalization of the plugin brand/name would make it easier to identify, even though it's a tag. But fine either way. +1 from me. Thanks!

Comment thread dashboard/src/components/Catalog/PackageCatalogItem.tsx Outdated
pkgPlugin = "flux";
break;
case PluginNames.PACKAGES_KAPP:
pkgPlugin = "carvel";
Copy link
Copy Markdown
Contributor

@absoludity absoludity Nov 11, 2021

Choose a reason for hiding this comment

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

Perhaps another update to the plugin API, which currently has the plugin details just with the machine-readable name (fluxv2.packages or kapp-controller.packages)... we could think about adding a displayname to avoid the dashboard needing to know about packaging plugins here.

Copy link
Copy Markdown
Contributor Author

@antgamdia antgamdia Nov 12, 2021

Choose a reason for hiding this comment

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

Yep, good idea indeed. I've added a card not to forget. Thanks!

@antgamdia
Copy link
Copy Markdown
Contributor Author

antgamdia commented Nov 12, 2021 •
edited
Loading

Oops, sorry, didn't remember 😅 btw, do you know if can I simulate it in chrome with any of these options?
image
I'm really willing to ensure the UX is the best for everyone!

Only other thought, as I mentioned on our other chat, I wonder if capitalization of the plugin brand/name would make it easier to identify, even though it's a tag. But fine either way. +1 from me. Thanks!

Mmm, interesting suggestion. Let me give you two proposals: small-caps and capitalize:

  1. current

image

  1. small-caps

image

  1. capitalize all

image

  1. capitalize the first character

image

I'd go with the current one, everything lowercased (0) or even (3), but no strong opinion here.

cc @ppbaena if you want to give your opinion as well

@antgamdia
Improve icon positioning
d0cbc55 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Use installPkg ready status
5ca8f2c 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
antgamdia
antgamdia commented Nov 12, 2021
View reviewed changes
Comment thread dashboard/src/components/AppList/AppListItem.tsx
description={app.shortDescription}
tag1Content={appStatus}
tag1Class={appStatus === "deployed" ? "label-success" : "label-warning"}
tag1Class={appReady ? "label-success" : "label-warning"}
Copy link
Copy Markdown
Contributor Author

@antgamdia antgamdia Nov 12, 2021

Choose a reason for hiding this comment

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

Squeezing this change in this PR, just for having a pkg-agnostic CSS here as well :P

Copy link
Copy Markdown
Contributor Author

@antgamdia antgamdia Nov 12, 2021

Choose a reason for hiding this comment

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

And... squeezing also this other change (cdeead4) for a multi-plugin header subtitles:

image

Copy link
Copy Markdown
Contributor

@absoludity absoludity Nov 14, 2021

Choose a reason for hiding this comment

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

That looks much clearer... ah, but this is just on the page header for a package detail? At first I thought you'd added the subtitle on the summary badge. But either way, this is excellent, much clearer.

@antgamdia
Extract getName fn to the utils file
d28d2fd 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Add multiplugin header subtitle
cdeead4 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Run linter
085100a 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia antgamdia changed the title Add plugin badge in the catalog Add plugin badge in the catalog + minor UI fixes for multiplugin Nov 12, 2021
@absoludity
Copy link
Copy Markdown
Contributor

absoludity commented Nov 14, 2021 •
edited
Loading

Oops, sorry, didn't remember sweat_smile btw, do you know if can I simulate it in chrome with any of these options? image I'm really willing to ensure the UX is the best for everyone!

I don't. I don't see any numbers in the ishihara 9 test but see plenty of colour there, And it's no problem, I was just trying to say that I'm not a good person to comment on colours, that's all.

Only other thought, as I mentioned on our other chat, I wonder if capitalization of the plugin brand/name would make it easier to identify, even though it's a tag. But fine either way. +1 from me. Thanks!

Mmm, interesting suggestion. Let me give you two proposals: small-caps and capitalize:

[snip]

I'd go with the current one, everything lowercased (0) or even (3), but no strong opinion here.

Yeah, I'd go with the current one or (4) (first letter capitalized), but don't care either. Note, the suggestion wasn't because of how the word "helm" looked on its own, but rather how it compares to the other tags, in particular, whether it stands out as the package type when next to other tags such as "marketplace". But the colour difference that you did is good for that, I'm sure.

antgamdia
antgamdia commented Nov 15, 2021
View reviewed changes
Comment thread dashboard/src/shared/utils.ts
Comment on lines +117 to +122
case PluginNames.PACKAGES_HELM:
return "Helm";
case PluginNames.PACKAGES_FLUX:
return "Flux";
case PluginNames.PACKAGES_KAPP:
return "Carvel";
Copy link
Copy Markdown
Contributor Author

@antgamdia antgamdia Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see any numbers in the ishihara 9 test

I can replicate it with protanopia (no red) and deuteranopia (no green), so next time I'll double-check all the UI decisions with (at least) these two options.

(4) (first letter capitalized),

Done :) Thanks for your comments. Merging!

Copy link
Copy Markdown
Contributor

@absoludity absoludity Nov 15, 2021

Choose a reason for hiding this comment

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

I don't see any numbers in the ishihara 9 test

I can replicate it with protanopia (no red) and deuteranopia (no green), so next time I'll double-check all the UI decisions with (at least) these two options.

Heh - Thanks Antonio, but I've never noticed any disadvantage caused by it.

@antgamdia antgamdia merged commit fe5c838 into vmware-tanzu:master Nov 15, 2021
@antgamdia antgamdia deleted the icons-2 branch November 15, 2021 12:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@absoludity absoludity absoludity approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@antgamdia @absoludity