Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: add chart companion components #139

Merged
merged 10 commits into from
Apr 27, 2019
Merged

feat: add chart companion components #139

merged 10 commits into from
Apr 27, 2019

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Apr 25, 2019

🏆 Enhancements

  • Create new package @superset-ui/chart-companion which includes the small things that are used to composite a chart.
  • Code are migrated from @superset-ui/plugins 's preset-chart-xy
  • Add unit tests for 100% coverage

@kristw kristw requested a review from a team as a code owner April 25, 2019 01:11
@netlify
Copy link

netlify bot commented Apr 25, 2019

Deploy preview for superset-ui ready!

Built with commit abc0e4b

https://deploy-preview-139--superset-ui.netlify.com

@codecov
Copy link

codecov bot commented Apr 25, 2019

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #139   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          77     81    +4     
  Lines         988   1039   +51     
  Branches      238    258   +20     
=====================================
+ Hits          988   1039   +51
Impacted Files Coverage Δ
...set-ui-chart-composition/src/legend/WithLegend.tsx 100% <100%> (ø)
...-ui-chart-composition/src/tooltip/TooltipTable.tsx 100% <100%> (ø)
...-ui-chart-composition/src/tooltip/TooltipFrame.tsx 100% <100%> (ø)
...s/superset-ui-chart-composition/src/ChartFrame.tsx 100% <100%> (ø)

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 58d56d5...abc0e4b. Read the comment docs.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me overall, had a few comments 👍

Name wise I sort of like chart-composition or chart-anatomy better than chart-companion, but if you feel strongly about companion we can go with that. @conglei wdyt?


#### API

`fn(args)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we update exports or skip for now?

import { 
  ChartFrame, 
  TooltipFrame, 
  TooltipTable, 
  WithLegend,
} from '@superset-ui/chart-companion';

flexDirection: this.getContainerDirection(),
};

style.width = width;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just set these when you define style? const style = { ..., width, height };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah true. delete the if condition that is no longer needed and forgot to look around.
will fix.

import React, { CSSProperties, ReactNode, PureComponent } from 'react';
import { ParentSize } from '@vx/responsive';
// eslint-disable-next-line import/no-unresolved
import * as CSS from 'csstype';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you just import FlexDirectionProperty?


triggerResizeObserver();
// Have to delay > 300ms
// because the default debounce time for ParentSize is 300.
Copy link
Contributor

@williaster williaster Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could expose this prop to avoid this (and also enable more flexibility)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great comment!

@kristw
Copy link
Contributor Author

kristw commented Apr 26, 2019

chart-composition sounds good

@kristw kristw force-pushed the kristw--chart-companion branch from b8bad0e to abc0e4b Compare April 26, 2019 23:51
@kristw kristw merged commit a82faec into master Apr 27, 2019
@delete-merged-branch delete-merged-branch bot deleted the kristw--chart-companion branch April 27, 2019 00:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants