-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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: A newly connected database doesn't appear in the databases list if user connected database using the 'plus' button #20363
fix: A newly connected database doesn't appear in the databases list if user connected database using the 'plus' button #20363
Conversation
…if user connected database using the 'plus' button
Codecov Report
@@ Coverage Diff @@
## master #20363 +/- ##
==========================================
- Coverage 66.63% 66.53% -0.11%
==========================================
Files 1738 1739 +1
Lines 65078 65151 +73
Branches 6885 6901 +16
==========================================
- Hits 43367 43346 -21
- Misses 19963 20052 +89
- Partials 1748 1753 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 good! But I think that we should include a test with the plus button to make sure that it is working.
/testenv up |
@AAfghahi Ephemeral environment spinning up at http://34.213.204.113:8080. Credentials are |
LGTM! |
return <RightMenu setQuery={setQuery} {...props} />; | ||
}; | ||
|
||
class RightMenuErrorWrapper extends React.PureComponent<RightMenuProps> { |
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.
Can we add some comments here? Others may be curious as to why an additional error wrapper is needed.
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
The RightMenu currently uses the
useQueryParam
hook that needs a Provider to wrap it (much like the redux store provider, theme provider, etc).Currently, we have more that one entry point (App.tsx/js) and they don't have the same structure.
Long term solution is to unify these into a single entry point, or at least, have the same provider structure for commonly used tools.
This PR enables the useQueryParam in said component if the router is available, and otherwise ignores it.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
--
TESTING INSTRUCTIONS
Ensure that, adding a database from the + menu works.
Ensure the explore loads correctly.
ADDITIONAL INFORMATION