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

update icons #18767

Merged
merged 3 commits into from
Jul 12, 2023
Merged

update icons #18767

merged 3 commits into from
Jul 12, 2023

Conversation

vndroid
Copy link
Contributor

@vndroid vndroid commented May 31, 2023

Replace the icon with a transparent background to adapt to Chrome or Firefox with different colors or custom themes.

1685500094028

The effect is shown in the picture, the left side is the original, and the right side is updated, you can see that it is more friendly for the different colors or custom themes.

@vndroid vndroid requested a review from a team as a code owner May 31, 2023 02:30
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #18767 (63178a0) into main (93e428d) will decrease coverage by 22.73%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #18767       +/-   ##
===========================================
- Coverage   67.39%   44.67%   -22.73%     
===========================================
  Files         985      236      -749     
  Lines      107969    13178    -94791     
  Branches     2698     2698               
===========================================
- Hits        72770     5887    -66883     
+ Misses      31298     6997    -24301     
+ Partials     3901      294     -3607     
Flag Coverage Δ
unittests 44.67% <ø> (-22.73%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 753 files with indirect coverage changes

@vndroid
Copy link
Contributor Author

vndroid commented Jun 7, 2023

@Vad1mo

@AllForNothing
Copy link
Contributor

AllForNothing commented Jun 7, 2023

@vndroid Thanks for the PR.
I will ask our UI designer to have a look at this.

@AllForNothing
Copy link
Contributor

@vndroid The only change is the bg-color(from white to transparent), right?

@vndroid
Copy link
Contributor Author

vndroid commented Jun 9, 2023

@vndroid The only change is the bg-color(from white to transparent), right?

Right.

Copy link
Contributor

@AllForNothing AllForNothing left a comment

Choose a reason for hiding this comment

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

LGTM

@AllForNothing
Copy link
Contributor

@vndroid
The change looks good, but I'm not sure if we have the right to modify this icon as the icon belongs to CNCF.https://cncf-branding.netlify.app/projects/harbor

@OrlinVasilev
Any suggestion for this?

Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

hold for discussing

@OrlinVasilev
Copy link
Member

Already addressed to CNCF if we need to follow some standard or something! maybe we can discuss it on the next CM

@Vad1mo Vad1mo removed the DONT MERGE label Jul 12, 2023
@Vad1mo Vad1mo requested a review from wy65701436 July 12, 2023 13:48
Copy link
Member

@OrlinVasilev OrlinVasilev left a comment

Choose a reason for hiding this comment

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

CNCF is ok with the change as does not break any branding or color schemas! CNCF service desk : CNCFSD-1848

@OrlinVasilev OrlinVasilev merged commit 06c4c1c into goharbor:main Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/update Update or Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants