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

Migrate Chart visualization to React Part 2: Editor #4139

Merged
merged 34 commits into from
Nov 20, 2019

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Sep 13, 2019

What type of PR is this? (check all applicable)

  • Refactor

Description

  • Convert component to React
    • General tab
    • X-Axis tab
    • Y-Axis tab
    • Series tab
    • Colors tab
      • for Pie
      • for Heatmap
      • for other types
    • Data Labels tab
    • Custom chart settings
  • Refine & performance
    • Debounce inputs
  • Cleanup
  • Tests

To discuss:

  • Custom chart: Auto update graph options (seems it never worked)
    • Keep as is for now, fix (re-implement) later.
  • Are X and Y Axis tabs needed for Pie chart type?
    • Not needed, now are hidden when global series type is pie.
  • Use Antd icons for chart types (seems there are all icons we need)
    • Keep as is for now.

Related Tickets & Documents

#3301 (Migrate Visualizations to React -> Chart)

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

There are some screenshots to show general look&feel. I didn't attach screenshots for all possible settings combinations.

Click to expand

General:

image

X Axis:

image

Y Axis:

All except of heatmap:

image

Heatmap:

image

Series:

image
image

Colors:

image

Data Labels:

image

@kravets-levko kravets-levko marked this pull request as ready for review September 23, 2019 13:49
@kravets-levko
Copy link
Collaborator Author

@arikfr @ranbena @gabrieldutra I still need to add some tests for chart editor, but meanwhile you're welcome to review the code and discuss items I mentioned in PR description. Thanks!

@arikfr
Copy link
Member

arikfr commented Oct 16, 2019

Probably not so hard to fix it, but it's rather implementing this feature from scratch - which I'd prefer to do as a new PR.

Ok, leave as is.

I think I'll hide that tabs when selecting pie as global series type.

👍

As for the fonts: whatever feels better to you.

@deecay
Copy link
Contributor

deecay commented Oct 20, 2019

Hi,

In this deploy preview page, "Edit Visualization", then go to "Colors" to change the color will unexpectedly change the bar chart into a line chart.

@kravets-levko
Copy link
Collaborator Author

Hey @deecay, nice catch 🙂 Thank you!

Copy link
Contributor

@ranbena ranbena left a comment

Choose a reason for hiding this comment

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

Got a few comments/suggestions in there

@kravets-levko kravets-levko requested a review from ranbena October 24, 2019 10:05
Copy link
Contributor

@ranbena ranbena left a comment

Choose a reason for hiding this comment

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

🎉

@susodapop
Copy link
Contributor

General behavior

This works as I expect it to.

Missing and NULL values

  1. "keep" should at least be capitalized "Keep"
  2. "convert to 0" should be capitalized "Convert To 0"

Can we change this text to make it more clear?

  1. "Do Not Display In Chart"
  2. "Convert To 0 And Display In Chart"

Color Picker

Tremendous addition. It would be nice if it remembered a pallet of recent colors.

@arikfr
Copy link
Member

arikfr commented Nov 20, 2019

Tremendous addition. It would be nice if it remembered a pallet of recent colors.

That's a good idea, but out of scope for this PR.

@kravets-levko kravets-levko merged commit 818649b into master Nov 20, 2019
@arikfr
Copy link
Member

arikfr commented Nov 20, 2019

🎉

@guidopetri guidopetri deleted the migrate-chart-editor branch July 22, 2023 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants