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(partition): linkLabel textColor override #1498

Merged
merged 6 commits into from
Dec 1, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Nov 22, 2021

Summary

The partition chart type now respects the textColor defined on the config.linkLabels object. This will NOT adjust the color based on the background contrast.

Screen Recording 2021-11-22 at 12 28 55 PM

Details

Added new ColorVariant key called Adaptive to be used in cases where color should be adaptive depending on the background contrast. Only when the the linkLabel.textColor is set to Adaptive, is the color contrast determined, otherwise the custom color string is used for all themes.

Also changed the eachTheme api to pass params without the & prefix, to be placed anywhere in the url.


Side note: I improved the error handling in the vrt framework. Before we waited for element from selector, if this failed, it would throw a misleading error that didn't help. Now if the element or url fails to load, there is an assertion to check the element exists and if not the test fails but does NOT throw an error. If the url fails to load the url will be logged to the console for easier debugging. See example below.

    fillLabel textcolor - treemap
      ✕ should show custom red textColor (10008 ms)

Docker Chromium: Shutting down Docker container...
  console.error
    Failed to load url. Check story at:

    	storybook url: http://localhost:9001/?path=/story/treemap--one-layer-2&knob-partitionLayout=sunburst&knob-custom fillLabel.textColor=true&knob-fillLabel.textColor=rgba(255, 0, 0, 0.85)
    	local vrt url: http://host.docker.internal:9002?path=%2Fstory%2Ftreemap--one-layer-2&knob-partitionLayout=sunburst&knob-custom%20fillLabel.textColor=true&knob-fillLabel.textColor=rgba(255%2C%200%2C%200%2C%200.85)&knob-debug=false

      441 |         return true;
      442 |       } catch {
    > 443 |         console.error(`Failed to load url. Check story at: \n\n\t storybook url: ${url}\n\tlocal vrt url: ${cleanUrl}`);
          |                 ^
      444 |         return false;
      445 |       }
      446 |     }

      at CommonPage.<anonymous> (page_objects/common.ts:443:17)
      at step (page_objects/common.ts:51:23)
      at Object.throw (page_objects/common.ts:32:53)
      at rejected (page_objects/common.ts:24:65)

Issues

fix #1469

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • The proper documentation and/or storybook story has been added or updated
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@nickofthyme nickofthyme added bug Something isn't working :styling Styling related issue :partition Partition/PieChart/Donut/Sunburst/Treemap chart related labels Nov 22, 2021
@nickofthyme
Copy link
Collaborator Author

FYI I came across #1499 while making these changes. So I added a VRT to test that in the future.

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Thanks Nick, this change works for me. After removing the textInvertible property from the config some release ago, there was no more option to overwrite that computed value.
What do you think if we use the same technique also for the fill_label?

@nickofthyme
Copy link
Collaborator Author

What do you think if we use the same technique also for the fill_label?

Yeah I can add that

@nickofthyme nickofthyme requested a review from markov00 November 23, 2021 16:47
@nickofthyme
Copy link
Collaborator Author

Added suggested change in 74013ff. @markov00 could you do another review of the added changes please, thanks!

@nickofthyme nickofthyme merged commit 3013310 into elastic:master Dec 1, 2021
@nickofthyme nickofthyme deleted the fix-link-label-style branch December 1, 2021 18:14
nickofthyme added a commit to nickofthyme/elastic-charts that referenced this pull request Dec 1, 2021
# Conflicts:
#	integration/tests/partition_stories.test.ts
nickofthyme pushed a commit that referenced this pull request Dec 1, 2021
## [40.0.1](v40.0.0...v40.0.1) (2021-12-01)

### Bug Fixes

* **partition:** linkLabel textColor override ([#1498](#1498)) ([#1515](#1515)) ([67c4eee](67c4eee))
nickofthyme pushed a commit that referenced this pull request Dec 9, 2021
# [40.2.0](v40.1.0...v40.2.0) (2021-12-09)

### Bug Fixes

* **partition:** linkLabel textColor override ([#1498](#1498)) ([3013310](3013310))
* **waffle:** use descend sortPredicate by default ([#1510](#1510)) ([763e2e3](763e2e3))
* **xy:** stacked polarity ([#1502](#1502)) ([920666a](920666a)), closes [#1280](#1280)

### Features

* **xy:** expose style for interpolation fit functions ([#1505](#1505)) ([3071457](3071457))
nickofthyme added a commit to nickofthyme/elastic-charts that referenced this pull request Dec 9, 2021
nickofthyme pushed a commit that referenced this pull request Dec 9, 2021
## [40.1.1](v40.1.0...v40.1.1) (2021-12-09)

### Bug Fixes

* **partition:** linkLabel textColor override ([#1498](#1498)) ([#1523](#1523)) ([5324c76](5324c76))
* **xy:** stacked polarity ([#1502](#1502)) ([#1522](#1522)) ([bfb853f](bfb853f)), closes [#1280](#1280)
nickofthyme added a commit to nickofthyme/elastic-charts that referenced this pull request Dec 10, 2021
# Conflicts:
#	integration/tests/axis_stories.test.ts
#	integration/tests/partition_stories.test.ts
#	packages/charts/src/chart_types/partition_chart/layout/viewmodel/link_text_layout.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working :partition Partition/PieChart/Donut/Sunburst/Treemap chart related :styling Styling related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Partition] Link label text color parameter passed is ignored
2 participants