refactor: Use Antd Dropdown instead of react-bootstrap in DatasourceControl #11395
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11395 +/- ##
==========================================
+ Coverage 65.79% 66.49% +0.70%
==========================================
Files 838 860 +22
Lines 39903 40871 +968
Branches 3662 3686 +24
==========================================
+ Hits 26253 27177 +924
- Misses 13549 13593 +44
Partials 101 101
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -125,6 +117,15 @@ class DatasourceControl extends React.PureComponent { | |||
| })); | |||
| } | |||
|
|
|||
| handleMenuItemClick({ key }) { | |||
| if (key === '1') { | |||
There was a problem hiding this comment.
Since we are already here, can you make these keys more human readable and maybe use constants?
| <Dropdown | ||
| overlay={datasourceMenu} | ||
| trigger={['click']} | ||
| id="datasource_menu" |
There was a problem hiding this comment.
Maybe we can get rid of id since you added data-test?
There was a problem hiding this comment.
Actually there are some styles for that, in DatasourceControl.less. @kgabryje would you mind seeing if those styles are still necessary/relevant? If they are, this may be the right opportunity to bring them into the component with Emotion Theme variables wired in. One less LESS file!
There was a problem hiding this comment.
Done, 1 less file less 🙂
There was a problem hiding this comment.
Marking as changes requested, in hopes of addressing some feedback on the thread.
Namely, whether or not the ID that @ktmud noticed (thanks!) is needed, and if the LESS styles the ID brings in can be either removed or brought in via Emotion.
|
@kgabryje So it seems "Explore SQL Lab" has a hover underline, but the other two menu item doesn't. Not really a big deal, but still kind of bothers me. Could you take a look? |
|
@ktmud Ah right, "Explore" is underlined because it's an anchor. I'll fix it |
…ontrol (apache#11395) * Create Tooltip component * Refactor DatasourceControl * Bug fix * Lint fix * E2E test fix * Move menu item keys to constants * Remove LESS file * Test fix * Test fix * Lint fix
mistercrunch
added
🏷️ bot
SUMMARY
Replaces react-bootstrap Dropdown with Antd Dropdown. Also, I replaced react-bootstrap Tooltip with Antd Tooltip, as it wasn't compatible with Antd Dropdown. This PR also fixes issues #11378 and #11094.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: see the screenshots in linked issues.
After:
TEST PLAN
ADDITIONAL INFORMATION