Skip to content
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

feat(polar): Intent to ship polar type #2462

Closed
wants to merge 26 commits into from

Conversation

cmpark0126
Copy link

Issue

Details

image

  • Implementation of the initial support of polar type
    • Support rendering function and various options to draw the polar chart
    • Support mouse event (it is inspired by the implementation of the pie chart)
    • Add multiple test cases for the polar chart
    • Add multiple demo cases for the polar chart

@netil
Copy link
Member

netil commented Dec 15, 2021

Hi @cmpark0126, thanks for the contribution!
I looked up and found several points to be checked.

  • The tests are failing. All tests should be passing without any errors.
  • Polar's options aren't functioning properly.
    When each of below option is specified, there's no changes observed.
    • ploar.level.max
    • ploar.padAngle
    • ploar.padding
    • ploar.startingAngle
    • ploar.size.ratio
  • Missing type definition declaration for polar options
    Each of polar options, should be declared in types, where is used for code assist, type checking, etc. (Ref.)
    https://github.com/naver/billboard.js/tree/master/types

@coveralls
Copy link

coveralls commented Dec 22, 2021

Coverage Status

Coverage decreased (-0.4%) to 89.807% when pulling 665ff3a on cmpark0126:polarChart into d3d2e05 on naver:master.

@cmpark0126
Copy link
Author

cmpark0126 commented Dec 22, 2021

Hi @netil, thanks for the comment and sorry for the late reply.
I tried to reflect what you pointed out. Please check again.

  • I expect the test will be passed now.
  • Polar's options you mentioned above are functioning properly now. If not, please let me know again.
  • I tried to add type definition declaration for polar options.

If you find another problem, please comment again.
Thank you!

Won-Joon Lee and others added 24 commits January 5, 2022 10:37
Co-authored-by: HanungLee <woong102345@gmail.com>
…area by using fill-opacity(#23)

* feat(polar): implement mouse events

feat(polar): linting

feat(polar): import d3Select

* feat(polar): show level line behind area

Co-authored-by: Chunmyong Park <chunmyong.park@kaist.ac.kr>
* fix(polar): remove unecessary param in polar demo

* feat(polar): add polar char option demo

* feat(polar): add demo for polar size
* feat(polar): add doc comments

* fix(polar): fix doc example code
* feat(polar): add polar_startingAngle option

* docs(polar): add polar_startingAngle doc in option

* docs(polar): add StartingAngle demo

* feat(polar): add polar padding option

* docs(polar): add doc for polar padding option

* docs(polar): add demo for polar padding option

* feat(polar): add polar padAngle option

* docs(polar): add docs for polar padAngle option

* docs(polar): add padAngle demo
* refactor(polar): create inner radius per area

* fix(polar): follow linter
* test(polar): add test for polar chart

* test(polar): test whether arc is rendered

* test(polar): fix polar rendering test case

* test(polar): set level option of polar

* test(polar): check level options modified before

* test(polar): set option which is hidden to show

* refactor(polar): rename option

* feat(polar): add polar_size_ratio option

* docs(polar): add demo for polar size ratio

* test(polar): add test for resize (unpassed)

* skip: fix typo
* docs(polar): fix demo typo

* fix(polar): function config update on polar

* skip: follow lint

* test(polar): add size option test

* test(polar): enhance test cases
@cmpark0126
Copy link
Author

@netil How's this PR going? Do I need to fix something for this PR? Thank you!

@netil
Copy link
Member

netil commented Jan 5, 2022

Hi @cmpark0126, due to the prior issues it is taking some time to look into.
I'll check the PR after they're done :)

.padAngle(padAngle);

// remove all `g` tag from previous polar chart
polar.arcs.selectAll("g").data([])
Copy link
Member

Choose a reason for hiding this comment

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

IMO isn't good approach on every redraw, because removing all nodes and re appending isn't cost effective.
It should be only updating d attributes to reduce unnecessary node handling.

polar2

@netil
Copy link
Member

netil commented Jan 6, 2022

I did looking the basic functionality and noticed during the legend toggling, the shapes aren't transitioning.
If you take on Pie's case, it behaves on toggles with transitions.
Can you fix this?

@netil
Copy link
Member

netil commented Mar 7, 2022

After reviewing & trying fixing the issues remained, found many duplications and realized most of basic functionalities are similar with the pie type. I'm working on rewrite of polar implementation not based from this PR.

The rewrite will add minimal code necessary and most functionalities will depends from the pie, which will make reduce bundle size and the reliability which already were providing from the pie.

netil added a commit to netil/billboard.js that referenced this pull request Mar 15, 2022
Implmentation of polar type

Ref naver#2462
netil added a commit to netil/billboard.js that referenced this pull request Mar 15, 2022
Implementation of polar type

Ref naver#2462
@netil netil closed this in feca715 Mar 15, 2022
@netil
Copy link
Member

netil commented Mar 15, 2022

@cmpark0126 @HanungLee @Justin-Moon @sni-J
It took time to be merged. Many thanks for the contribution!

github-actions bot pushed a commit that referenced this pull request Mar 16, 2022
# [3.4.0-next.1](3.3.3...3.4.0-next.1) (2022-03-16)

### Bug Fixes

* **axis:** Fix culling visibility on dynamic loading ([4c79daf](4c79daf)), closes [#2582](#2582)
* **axis:** fix hidden axis rescale on dynamic load ([5418853](5418853)), closes [#2523](#2523) [#2571](#2571)
* **util:** Check if agent has mouse ([d42adaa](d42adaa)), closes [#2550](#2550) [#2585](#2585)

### Features

* **module:** Support dual CJS/ESM package ([437c007](437c007)), closes [#2202](#2202)
* **plugin:** Intent to ship TableView plugin ([215b611](215b611)), closes [#1873](#1873)
* **polar:** Intent to ship polar type ([feca715](feca715)), closes [#2462](#2462)
@cmpark0126
Copy link
Author

@netil
Sorry for the late reply. I apologize for delaying this work and thanks for the advice about this work and completing this PR.

github-actions bot pushed a commit that referenced this pull request Mar 31, 2022
# [3.4.0](3.3.3...3.4.0) (2022-03-31)

### Bug Fixes

* **api:** Fix flow on indexed/category axis type ([4aba436](4aba436)), closes [#2595](#2595)
* **axis:** Fix culling visibility on dynamic loading ([4c79daf](4c79daf)), closes [#2582](#2582)
* **axis:** fix hidden axis rescale on dynamic load ([5418853](5418853)), closes [#2523](#2523) [#2571](#2571)
* **generator:** Prevent possible infinite loop when tab isn't visible ([bafdb17](bafdb17)), closes [#2606](#2606)
* **option:** Fix data.hide not working for bubble/scatter type ([64ae74b](64ae74b)), closes [#2609](#2609)
* **util:** Check if agent has mouse ([d42adaa](d42adaa)), closes [#2550](#2550) [#2585](#2585)
* **util:** Enhance parsing date string ([8d9f422](8d9f422)), closes [#1714](#1714)

### Features

* **api:** Implement axis range reset ([6c9d99e](6c9d99e)), closes [#2398](#2398)
* **option:** Intent to ship onclick ([63c5a53](63c5a53)), closes [#2587](#2587)
* **polar:** Intent to ship polar type ([feca715](feca715)), closes [#2462](#2462)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants