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

fix: fix box type data labels using useSeriesColor option #687

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

jwlee1108
Copy link
Contributor

@jwlee1108 jwlee1108 commented Jun 30, 2021

Please check if the PR fulfills these requirements

  • It's submitted to right branch according to our branching model
  • It's right issue type on title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has description for the breaking change

Description

image

There is a problem with dataLabels on Bar-like charts using useSeriesColor option. Even though it sets the option, colors of dataLabels aren't changed.


Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

Copy link

@lja1018 lja1018 left a comment

Choose a reason for hiding this comment

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

리뷰완료합니다.

Copy link

@jajugoguma jajugoguma left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다! 👏

Copy link
Member

@heenakwag heenakwag left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다.
PR 제목이나 커밋에 이슈 번호를 적지 않더라도, 설명에는 적는게 어떨까요? 어떤 문제 때문에 수정하는 것인지 알 수 없어서 리뷰하기가 조금 어렵네요.
개인적으로는 이슈가 닫히더라도 커밋이나 PR에 이슈 번호를 적는게 이슈 관리 측면에서 더 좋을 것 같아요.

그리고 이슈 번호 기입과 무관하게 PR 설명에는 관련 이슈에 대해 적어주시면 좋을 것 같습니다.

@@ -56,7 +56,7 @@
},
"apps/chart": {
"name": "@toast-ui/chart",
"version": "4.3.4",
"version": "4.3.5",
Copy link

Choose a reason for hiding this comment

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

버전은 기존 버전이 반영이 안되어 올리신건가요~?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저번에 배포 인수인계 하면서 apps/charts 쪽 버전은 확인했는데 전체 기준 npm install 하니까 이 부분 변경이 있었고 원래 해야 했던 건데 누락된거라 판단해서 같이 올렸어요.

Copy link

@js87zz js87zz left a comment

Choose a reason for hiding this comment

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

리뷰 완료입니다. 이전 리뷰에도 남겼는데 나중에 이력을 찾거나 컨트리뷰터들도 볼 수 있으니, PR 디스크립션에 어떤 작업을 했는지 좀 더 자세히 적으면 좋겠습니다~!

@jwlee1108 jwlee1108 merged commit a6ffd63 into main Jul 1, 2021
@jwlee1108 jwlee1108 deleted the fix/685/labels branch July 1, 2021 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants