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

Knative Service icon could contain a Knative logo. #200

Closed
wants to merge 5 commits into from

Conversation

sudhirverma
Copy link
Contributor

Fix: #196

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2022

Codecov Report

Merging #200 (e8a824d) into main (c152971) will decrease coverage by 0.20%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
- Coverage   92.99%   92.79%   -0.21%     
==========================================
  Files          74       74              
  Lines        2942     2942              
  Branches      468      468              
==========================================
- Hits         2736     2730       -6     
- Misses        206      212       +6     
Impacted Files Coverage Δ
src/eventingTree/eventingTreeItem.ts 100.00% <ø> (ø)
.../functions/function-tree-view/functionsTreeItem.ts 34.61% <ø> (ø)
src/util/platform.ts 88.23% <0.00%> (-11.77%) ⬇️
src/cli/cmdCli.ts 49.27% <0.00%> (-4.35%) ⬇️
src/util/watch.ts 93.33% <0.00%> (-3.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c152971...e8a824d. Read the comment docs.

@sudhirverma sudhirverma requested a review from lstocchi March 21, 2022 09:10
@lstocchi
Copy link
Contributor

Adding a comment on the issue, wait a sec before merging it please

@sudhirverma sudhirverma requested a review from lstocchi March 28, 2022 08:45
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Not sure if this can be made but it would be better if icons could be a bit bigger.
I have glasses so maybe this is my problem 😆 but i really don't see any difference if i sit normally in front of my screen. I need to get closer to read what's inside the labels.
image

Then maybe you can think about adding icons to the categories as well, as we do with tekton. Now there are folder icons. But this is your or Mohit choice
image

@mohitsuman
Copy link
Collaborator

@sudhirverma Can you paste the latest icon size and how it looks ?

@mohitsuman mohitsuman self-requested a review March 28, 2022 12:27
@sudhirverma
Copy link
Contributor Author

sudhirverma commented Mar 28, 2022

@lstocchi updated size

Screenshot 2022-03-28 at 5 51 36 PM

Should I change Brokers folder icon to labels B

@mohitsuman
Copy link
Collaborator

@sudhirverma We need to do the following changes:

@sudhirverma
Copy link
Contributor Author

@mohitsuman

Screenshot 2022-03-28 at 6 49 37 PM

@mohitsuman
Copy link
Collaborator

@sudhirverma KSVC and REV badge sizes are different. The UX doesn't feel good, try to make it a similar size.

@mohitsuman
Copy link
Collaborator

@sudhirverma please add the latest image after the fixes in the icon size.

@sudhirverma
Copy link
Contributor Author

sudhirverma commented Apr 4, 2022

@mohitsuman below image with SVG file

Screenshot 2022-04-04 at 5 12 00 PM

and below image with png file

Screenshot 2022-04-04 at 5 22 55 PM

@lstocchi
Copy link
Contributor

lstocchi commented Apr 5, 2022

Maybe better with SVG. They don't look very nice but i guess vscode doesn't allow to have bigger icons?? What i really don't like (even in IJ) is the different style between the namespace icon and the rest.

BTW I am not even sure we are doing the right thing about removing the old icons.... Joshua did great from my pov.

@mohitsuman
Copy link
Collaborator

Looking at the limitation we have on vscode regarding icon size and how four character string inside a badge does not align with the UI, we are not going to merge these changes. We will be using the same set of icons used by Knative service, eventing and broker as it was approved by knative upstream community.

@mohitsuman mohitsuman closed this Apr 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Knative Service icon could contain a Knative logo.
4 participants