appsmith icon indicating copy to clipboard operation
appsmith copied to clipboard

Published 20 hours ago • verified publisher icon appsmithorg

feat: Add Series Colour property for default charts

Open souma-ghosh opened this issue 2 years ago • 14 comments

Description

In default charts, only series title and series data could be provided by developer. We have added the Series Color property as well in this PR to control the color of all the default charts (Except Pie Chart).

  • Not applicable for custom charts
  • Series Color will only work with hex code values as of now
  • Series Color not applicable for Pie Chart and hence this feature is not available for Pie Chart

Fixes #15522

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Manual

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:

Dev activity

  • [x] My code follows the style guidelines of this project
  • [x] I have performed a self-review of my own code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes
  • [ ] PR is being merged under a feature flag

QA activity:

  • [ ] Test plan has been approved by relevant developers
  • [ ] Test plan has been peer reviewed by QA
  • [ ] Cypress test cases have been added and approved by either SDET or manual QA
  • [ ] Organized project review call with relevant stakeholders after Round 1/2 of QA
  • [ ] Added Test Plan Approved label after reveiwing all Cypress test

souma-ghosh avatar Nov 17 '22 10:11 souma-ghosh

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview Dec 2, 2022 at 8:40AM (UTC)

vercel[bot] avatar Nov 17 '22 10:11 vercel[bot]

Unable to find test scripts. Please add necessary tests to the PR.

github-actions[bot] avatar Nov 17 '22 10:11 github-actions[bot]

Deployment failed with the following error:

Resource is limited - try again in 7 minutes (more than 100, code: "api-deployments-free-per-day").

vercel[bot] avatar Nov 23 '22 10:11 vercel[bot]

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

github-actions[bot] avatar Nov 30 '22 16:11 github-actions[bot]

Unable to find test scripts. Please add necessary tests to the PR.

github-actions[bot] avatar Dec 01 '22 10:12 github-actions[bot]

Unable to find test scripts. Please add necessary tests to the PR.

github-actions[bot] avatar Dec 01 '22 10:12 github-actions[bot]

Observation: @souma-ghosh cc: @dilippitchika

  • Color applied on charts is not same as actual color on adding less/more than 6 digit alphanumeric color code in Series Color property https://bthrujcsw8.vmaker.com/record/7JzCerfGDvBVXMHJ

chandannkumar avatar Dec 06 '22 09:12 chandannkumar

Observation: @souma-ghosh cc: @dilippitchika

  • Color applied on charts is not same as actual color on adding less/more than 6 digit alphanumeric color code in Series Color property https://bthrujcsw8.vmaker.com/record/7JzCerfGDvBVXMHJ

This is happening because Fusion chart interprets hex colours differently when the hex is less than 6 digits. This is unpredictable as far as I tested. So we can expect this to work predictably when the series colour hex is 6 characters. Also fusion chart does not support text colours like "red", "green" etc. so the option has been limited to hex code. We can put some validation to enforce the hex colours to be 6 characters only or think about switching to a different chart widget which can reliably interpret the colours provided. @dilippitchika Could you comment on whether we can move ahead with this?

souma-ghosh avatar Dec 07 '22 07:12 souma-ghosh

Ok this is a limitation, please create a new bug for it. Also @souma-ghosh can we support JS in the field?

dilippitchika avatar Dec 07 '22 07:12 dilippitchika

@dilippitchika ATM we can't. Since we have implemented the property controls for Chart Series in a bit hacky way. So we will need a refactor there before we can make the Series Color field JS convertible.

souma-ghosh avatar Dec 07 '22 08:12 souma-ghosh

Understood, can you please create a new issue for it?

dilippitchika avatar Dec 07 '22 09:12 dilippitchika

Here is the issue https://github.com/appsmithorg/appsmith/issues/18745

souma-ghosh avatar Dec 07 '22 09:12 souma-ghosh

/ok-to-test sha=a199af5

souma-ghosh avatar Dec 07 '22 11:12 souma-ghosh

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3638524348. Workflow: Appsmith External Integration Test Workflow. Commit: a199af5. PR: 18229. Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18229&runId=3638524348_1

github-actions[bot] avatar Dec 07 '22 11:12 github-actions[bot]

/ok-to-test sha=a199af5

souma-ghosh avatar Dec 08 '22 06:12 souma-ghosh

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3646012301. Workflow: Appsmith External Integration Test Workflow. Commit: a199af5. PR: 18229. Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18229&runId=3646012301_1

github-actions[bot] avatar Dec 08 '22 06:12 github-actions[bot]

Test Plan for Series Colour property of Chart series https://github.com/appsmithorg/TestSmith/issues/2115

chandannkumar avatar Dec 09 '22 05:12 chandannkumar