feat(control-utils): add infotooltipwithtrigger #442

pkdotson · 2020-05-05T23:57:41Z

🏆 Enhancements

Adding infotooltipwithtrigger to superset ui. This is required for migrating partition chart control panels.

vercel · 2020-05-05T23:57:44Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/73baqpif1
✅ Preview: https://superset-ui-git-fork-preset-io-phillip-so-264-partiti-6351f5.superset.now.sh

codecov · 2020-05-06T01:52:39Z

Codecov Report

Merging #442 into master will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #442      +/-   ##
=========================================
+ Coverage    3.08%   3.20%   +0.12%     
=========================================
  Files         151     152       +1     
  Lines        5218    5238      +20     
  Branches      273     275       +2     
=========================================
+ Hits          161     168       +7     
- Misses       5020    5033      +13     
  Partials       37      37

Impacted Files	Coverage Δ
plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js	`0.00% <0.00%> (ø)`
plugins/legacy-plugin-chart-sunburst/src/index.js	`0.00% <0.00%> (ø)`
plugins/legacy-plugin-chart-world-map/src/index.js	`0.00% <0.00%> (ø)`
...ins/legacy-preset-chart-nvd3/src/transformProps.js	`0.00% <0.00%> (ø)`
...ins/legacy-preset-chart-nvd3/src/utils/isTruthy.js	`100.00% <0.00%> (ø)`
...ins/legacy-preset-chart-nvd3/src/utils/tokenize.ts	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86847d1...1c4be62. Read the comment docs.

ktmud

Thanks for doing this. Added a couple of comments on style nits.

I'm not sure we want to put this under control-utils, though, as this seems to be a fairly generic component that is used in more than chart controls.

I feel we kind of need a generic UI library + an arsenal of library-agnostic chart controls to better prepare for future migration.

ktmud · 2020-05-06T02:16:05Z

packages/superset-ui-control-utils/package.json

+  "dependencies": {
+    "lodash": "^4.17.15",
+    "react": "^16.13.1",
+    "react-bootstrap": "^1.0.1"


Re: dependencies

These should be peerDependencies, as they are shared by multiple superset-ui packages and should be upgraded all at once.

We are not using react-bootstrap@1 yet. It's for Bootstrap v4, but we are still stuck with Boostrap v3. Check incubator-superset for property versions for react-bootstrap and @types/react-bootstrap.

Can we get rid of lodash? Is kebabCase absolutely necessary? Could you investigate why do we needed an ID for the tooltip in the first place? It's the best if we could just remove it. Less dependencies the better.

Regarding 3, I think that kind of effort is better left until after we've moved things from superset. This should be as clean a refactor as possible, just moving code into superset-ui.

react in particular should always be peerDependencies in any library. There should always be one react.

react-bootstrap can be left in dependencies

I also don't agree with making these peer dependencies, because the code directly depends on them. Peer dependencies aren't meant to be used for optimization, npm does that automatically when version numbers of dependencies are compatible with each other.

I agree with making them dependencies as-is except react

True, npm might be able to remove duplicates, but I think there is also the implication of what if it is not compatible? Are we really OK with having multiple versions of the dependent libraries in our app?

These superset-ui packages cannot work outside of Superset or Superset UI demos, so they should not introduce their own large non-optional dependencies. Using peerDependencies kind of forces package maintainers and library users to keep the dependencies up to date.

True, npm might be able to remove duplicates, but I think there is also the implication of what if it is not compatible? Are we really OK with having multiple versions of the dependent libraries in our app?

I think that is how all packages work tho. I am ok having multiple versions of the dependent instead of worrying about breaking anything if bump version of one dependencies.

These superset-ui packages cannot work outside of Superset or Superset UI demos, so they should not introduce their own large non-optional dependencies. Using peerDependencies kind of forces package maintainers to keep the dependencies up to date.

This is actually not entirely correct. These packages can work outside of Superset and Superset UI demos. They are independent packages on their own (number format, time format, color, etc.) which do not need Superset to function. I have them as dependencies in other projects and have always think of them independent from incubator-superset. They should have their own dependencies.

All other superset-ui packages have things (including lodash) listed under dependencies. Is this different somehow?

I am proposing we proceed with moving react to peerDependencies and leave the remaining in dependencies unless there is objection?

No objection here. I actually already moved react, but haven't pushed it yet.

ktmud · 2020-05-06T02:17:50Z

packages/superset-ui-control-utils/src/InfoTooltipWithTrigger.tsx

+  icon: 'info-circle',
+  placement: 'right',
+};
+const tooltipStyle = { wordWrap: 'break-word' } as any;


Try avoid any if possible:

const tooltipStyle: React.CSSProperties = { wordWrap: 'break-word' };

Since this rule is so simple, you can probably also just inline the style in <Tooltip ... style={}>.

having constant here is better so react won't re-render. when passing object to style, react only compares reference.

ktmud · 2020-05-06T02:25:29Z

packages/superset-ui-control-utils/src/InfoTooltipWithTrigger.tsx

+import React from 'react';
+import { kebabCase } from 'lodash';
+// @ts-ignore
+import { Tooltip, OverlayTrigger } from 'react-bootstrap';


Prefer to remove // @ts-ignore and add proper typing support by installing @types/react-bootstrap.

If I remember correctly, bootstrap 3 doesn't have types support. We ran into this problem in incubator-superset as well.

Instead of @ts-ignore, you can add a declaration file. Even if it is an empty declaration

Example:
https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-translation/types/external.d.ts

I think @types/react-bootstrap @ 0.32.2 works with Bootstrap v3. There might be cases with suboptimal typings, where you can then do @ts-ignore per module.

Nice! That means we can add types for bootstrap back in incubator-superset too

@ktmud I tried adding the types for bootstrap, but that leads to an issue in CI. It looks like our version of typescript isn't happy with those type definitions, any ideas on how to fix?

Sorry, should've used @types/react-bootstrap @ ^0.32.21. This is also what's used in incubator-superset.

Btw, it's preferable to have @types/* in dependencies instead of devDependencies so that dependent packages can have easier access to typings when the library is used in npm link.

ktmud · 2020-05-06T02:29:05Z

packages/superset-ui-control-utils/src/InfoTooltipWithTrigger.tsx

+  className: 'text-muted',
+  icon: 'info-circle',
+  placement: 'right',
+};


Prefer to use ES6 function argument defaults.

ktmud · 2020-05-06T02:29:54Z

packages/superset-ui-control-utils/src/InfoTooltipWithTrigger.tsx

+  placement: string;
+  bsStyle: string;
+  className: string;
+}


Make those with defaults optional.

interface Props { label: string; icon: string; tooltip?: string; onClick?: () => void; placement?: string; bsStyle?: string; className?: string; }

+1 @ktmud with the small change to use type for Props instead of interface. (Just convention from guidelines at Airbnb and used throughout the codebase at the moment). Also export the type.

interface means the properties are immutable which often runs into problems when you want to extend the component and override the prop types.

packages/superset-ui-control-utils/src/InfoTooltipWithTrigger.tsx

kristw · 2020-05-06T21:30:01Z

Please also add unit test to ensure 100% coverage

suddjian · 2020-05-07T18:14:30Z

Addressed PR feedback, should be ready for a re-review

rusackas

LGTM! Thanks to all involved for giving this so much due diligence.

PR to merge packges for controls migration

5dc5d23

pkdotson requested a review from a team as a code owner May 5, 2020 23:57

vercel bot had a problem deploying to Preview May 5, 2020 23:57 Failure

pull-request-size bot added the size/M label May 5, 2020

pkdotson changed the title ~~PR to merge packges for controls migration~~ chore: PR to merge packges for controls migration May 5, 2020

fix: add bootstrap

5efe029

pull-request-size bot added size/XS and removed size/M labels May 6, 2020

vercel bot had a problem deploying to Preview May 6, 2020 00:13 Failure

fix: change tooltip to tsx

9acf813

pull-request-size bot removed the size/XS label May 6, 2020

vercel bot had a problem deploying to Preview May 6, 2020 00:36 Failure

pull-request-size bot added the size/M label May 6, 2020

fix: types

bf1296b

vercel bot had a problem deploying to Preview May 6, 2020 00:54 Failure

fix: tsx error

00561e3

vercel bot deployed to Preview May 6, 2020 01:12 View deployment

appease linter, enhance a11y

ed63e91

vercel bot deployed to Preview May 6, 2020 01:43 View deployment

ktmud reviewed May 6, 2020

View reviewed changes

kristw reviewed May 6, 2020

View reviewed changes

packages/superset-ui-control-utils/src/InfoTooltipWithTrigger.tsx Show resolved Hide resolved

addressing PR feedback

3fa07d2

vercel bot had a problem deploying to Preview May 6, 2020 22:43 Failure

pull-request-size bot added size/L and removed size/M labels May 6, 2020

tweaking bootstrap types

dc8cb7e

vercel bot deployed to Preview May 7, 2020 00:17 View deployment

fix test

7f27efe

vercel bot deployed to Preview May 7, 2020 06:06 View deployment

test enter key

694c63d

vercel bot deployed to Preview May 7, 2020 06:44 View deployment

moar tests

8cac2d1

vercel bot deployed to Preview May 7, 2020 16:33 View deployment

code > key

c0ce659

vercel bot had a problem deploying to Preview May 7, 2020 17:06 Failure

ugh fine

1c4be62

vercel bot deployed to Preview May 7, 2020 17:30 View deployment

rusackas approved these changes May 7, 2020

View reviewed changes

kristw changed the title ~~chore: PR to merge packges for controls migration~~ feat(control-utils): add infotooltipwithtrigger May 7, 2020

kristw merged commit 488e945 into apache-superset:master May 7, 2020

pkdotson mentioned this pull request May 7, 2020

feat: partitionchart controls migration apache/superset#9768

Merged

9 tasks

rusackas deleted the phillip/SO-264-partitionchart-controls-packages branch July 17, 2020 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(control-utils): add infotooltipwithtrigger #442

feat(control-utils): add infotooltipwithtrigger #442

pkdotson commented May 5, 2020 •

edited by kristw

Loading

vercel bot commented May 5, 2020 •

edited

Loading

codecov bot commented May 6, 2020 •

edited

Loading

ktmud left a comment •

edited

Loading

ktmud May 6, 2020

suddjian May 6, 2020

kristw May 6, 2020 •

edited

Loading

suddjian May 6, 2020

kristw May 6, 2020

ktmud May 6, 2020 •

edited

Loading

kristw May 6, 2020 •

edited

Loading

suddjian May 6, 2020 •

edited

Loading

kristw May 6, 2020

suddjian May 6, 2020

ktmud May 6, 2020

kristw May 6, 2020

ktmud May 6, 2020

suddjian May 6, 2020

kristw May 6, 2020

ktmud May 6, 2020

suddjian May 6, 2020

suddjian May 6, 2020

ktmud May 6, 2020

ktmud May 6, 2020

ktmud May 6, 2020

kristw May 6, 2020 •

edited

Loading

ktmud May 6, 2020

kristw commented May 6, 2020

suddjian commented May 7, 2020

rusackas left a comment

feat(control-utils): add infotooltipwithtrigger #442

feat(control-utils): add infotooltipwithtrigger #442

Conversation

pkdotson commented May 5, 2020 • edited by kristw Loading

vercel bot commented May 5, 2020 • edited Loading

codecov bot commented May 6, 2020 • edited Loading

Codecov Report

ktmud left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud May 6, 2020 • edited Loading

Choose a reason for hiding this comment

kristw May 6, 2020 • edited Loading

Choose a reason for hiding this comment

suddjian May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw commented May 6, 2020

suddjian commented May 7, 2020

rusackas left a comment

Choose a reason for hiding this comment

pkdotson commented May 5, 2020 •

edited by kristw

Loading

vercel bot commented May 5, 2020 •

edited

Loading

codecov bot commented May 6, 2020 •

edited

Loading

ktmud left a comment •

edited

Loading

kristw May 6, 2020 •

edited

Loading

ktmud May 6, 2020 •

edited

Loading

kristw May 6, 2020 •

edited

Loading

suddjian May 6, 2020 •

edited

Loading

kristw May 6, 2020 •

edited

Loading