-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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(dev-server run): Check for environment tag not being undefined #21232
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21232 +/- ##
==========================================
- Coverage 66.45% 66.45% -0.01%
==========================================
Files 1784 1784
Lines 68142 68141 -1
Branches 7264 7265 +1
==========================================
- Hits 45285 45284 -1
+ Misses 20989 20988 -1
- Partials 1868 1869 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@villebro can you please check this? |
@sinhashubham95 I tried as like your changes from your commit [https://github.com//pull/21232/commits/53f9f75d443bba7689a2a77473fed2e11b999e23] |
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!
@villebro @stephenLYZ can you merge this? |
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.
Minor nit, other than that LGTM
@@ -281,7 +281,7 @@ const RightMenu = ({ | |||
onDatabaseAdd={handleDatabaseAdd} | |||
/> | |||
)} | |||
{environmentTag.text && ( | |||
{environmentTag && environmentTag.text && ( |
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.
Could we simplify this to
{environmentTag && environmentTag.text && ( | |
{environmentTag?.text && ( |
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.
Oh, too late 🙁
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.
Ops...Let's put it on the next PR
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.
Ops...Let's put it on the next PR
no worries, it was just a minor nit 🙂👍
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.
Opened a PR to fix this once and for all.
SUMMARY
Environment tag is not set in the dev-server, so reference to a key in the environment tag was making the dev-server not loadable.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Try to load Superset on dev-server, it does not load. After this fix, it should load.
ADDITIONAL INFORMATION