-
Notifications
You must be signed in to change notification settings - Fork 356
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
fix: fix fallback logic for partially provided custom logos #9842
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9842 +/- ##
=======================================
Coverage 54.71% 54.72%
=======================================
Files 1261 1261
Lines 156015 156011 -4
Branches 3590 3589 -1
=======================================
+ Hits 85361 85372 +11
+ Misses 70522 70507 -15
Partials 132 132
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Left a few comments
0e19b10
to
0701e5d
Compare
5768284
to
9a3a4dc
Compare
9a3a4dc
to
5662c20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes LGTM! Just left a few comments about a docs change and some test case naming changes.
f1da88c
to
0f9fe26
Compare
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
result := tt.variations.PickVariation(tt.mode, tt.orientation) | ||
name := fmt.Sprintf("mode=%s, orientation=%s", tt.mode, tt.orientation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If mode
or orientation
== "", then these test names might be a bit confusing.
Should we do
name := fmt.Sprintf("mode=%s, orientation=%s", tt.mode, tt.orientation) | |
mode := "none" | |
if len(tt.mode) > 0 { | |
mode = tt.mode | |
} | |
orientation := "none" | |
if len(tt.orientation) > 0 { | |
orientation = tt.orientation | |
} | |
name := fmt.Sprintf("mode=%s, orientation=%s", mode, orientation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nit, so feel free to ignore!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Onee small change request and should be good after that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, awesome work!
Ticket
Description
we had a custom logic for doing fallback for variations of the logo that we needed from the user. here we simplify that and leave it to the user to decide to use duplicate vartions if desired.
Test Plan
logo_path
as well as one with some missing logo variationChecklist
docs/release-notes/
See Release Note for details.