-
-
Notifications
You must be signed in to change notification settings - Fork 781
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 issues with setting sky to undefined #4587
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #4587 +/- ##
==========================================
- Coverage 87.74% 87.63% -0.12%
==========================================
Files 247 247
Lines 33494 33504 +10
Branches 2414 2289 -125
==========================================
- Hits 29390 29361 -29
- Misses 3118 3164 +46
+ Partials 986 979 -7 ☔ View full report in Codecov by Sentry. |
|
I've solved the animation issue in last commit. |
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.
Code looks ok except this one comment.
As for the overall design, it's difficult for me to judge
break; | ||
if (!skyOptions && !sky) return; | ||
|
||
if (skyOptions && !sky) { |
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.
I think a little shorter and more readable would be
if (skyOptions && sky) {
for...
} else {
update = true
}
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.
But it's not enough - you need to check all 4 options...
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.
In any case, I think it's OK, if you feel strongly about it let me know...
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.
I think it's not a big deal )
Oops... enabled auto-merge... sorry... |
Launch Checklist
This is a fix to issues related to sky definition.
Fixes #4517
CHANGELOG.md
under the## main
section.