Kitware / candela

Visualization components for the web
https://candela.readthedocs.io
Apache License 2.0
116 stars 29 forks source link

Add SentenTree component #485

Closed waxlamp closed 7 years ago

waxlamp commented 7 years ago

sententree

Depends on #483 for proper CI.

codecov-io commented 7 years ago

Codecov Report

Merging #485 into master will increase coverage by 0.12%. The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #485      +/-   ##
==========================================
+ Coverage   33.99%   34.11%   +0.12%     
==========================================
  Files          38       39       +1     
  Lines        1518     1530      +12     
==========================================
+ Hits          516      522       +6     
- Misses       1002     1008       +6
Impacted Files Coverage Δ
components/index.js 100% <100%> (ø) :arrow_up:
components/SentenTree/index.js 45.45% <45.45%> (ø)

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 cbc653f...65695cf. Read the comment docs.

mgrauer commented 7 years ago

Can you add a screenshot if it's easy? I'm curious what this thing is.

🍛

waxlamp commented 7 years ago

@mgrauer, sure thing, screencap added to top comment.

waxlamp commented 7 years ago

Rebased on master.

jeffbaumes commented 7 years ago

@ronichoudhury this. is. cool.

Looks like this has conflicts now, probably pretty simple to rebase.

waxlamp commented 7 years ago

Rebased.

jeffbaumes commented 7 years ago

Looking good. I discovered that you are not emptying the div before rendering the component. Could you add that in?

waxlamp commented 7 years ago

Oops. Added in a new commit.

It might be worthwhile to put some of this stuff (emptying top-level div, prepping datasets with column selectors) into helper functions. I'll file an issue to that effect.

waxlamp commented 7 years ago

Is it better to leave it as integer and fix upstream, or change it to number so that it works now?

jeffbaumes commented 7 years ago

Since this is not on a critical path, I'd say leave as integer and support it as needed in downstream tools.