-
Notifications
You must be signed in to change notification settings - Fork 272
Conversation
Deploy preview for superset-ui ready! Built with commit 5b3b321 |
Deploy preview for superset-ui ready! Built with commit f950af6 |
(cherry picked from commit dd55fd8)
95a9e47
to
3be990b
Compare
Codecov Report
@@ Coverage Diff @@
## master #281 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 104 102 -2
Lines 1234 1212 -22
Branches 296 298 +2
=====================================
- Hits 1234 1212 -22
Continue to review full report at Codecov.
|
cc @etr2460 finally got a green CI |
finally looking at this, thanks for all the effort/pain 🙏 |
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 overall, had a few thoughts that could be worth fixing before merge. thanks again! 💖
@@ -35,8 +35,8 @@ export default class ChartFrame extends PureComponent<Props, {}> { | |||
}} | |||
> | |||
{renderContent({ | |||
height: Math.max(contentHeight || 0, height), | |||
width: Math.max(contentWidth || 0, width), | |||
height: Math.max(contentHeight ?? 0, height), |
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.
yesssssss
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.
@etr2460 added these internally and although the build passed it threw errors in the app. might be worth testing (I guess if preview works it should be good)
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 sounds scary. I checked the storybook previews and they work correctly.
What are the issues that appear in app? I could try to revert back to ||
and suppress eslint
if it is risky.
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.
you need to make sure that babel updated to handle the new syntax. babel 7.8.3 should do the trick.
🏠 Internal
nimbus
, which is a shareable build config forjest
,babel
,eslint
,prettier
, andtypescript
on top ofbeemo
.eslint
rules for test files at the moment.Notable overrides
timer
toreal
(jest
's default) instead ofnimbus
's default which usesfake
timer. For unknown reason,jest.useRealTimers()
has no effect if the defaulttimer
isfake
. On the other hand, if the defaulttimer
isreal
,jest.useFakeTimers()
works fine.eslint
from auto-fixing theno-explicit-any
rule and convert allany
into (the)unknown
. 🎶❄️