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.

Sign up for GitHub

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

Jump to bottom

Set toolbar control background to transparent #858

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Set toolbar control background to transparent #858

merged 1 commit into from
Nov 10, 2020

Conversation

hlyang397
Copy link
Contributor

Updates

  • use fill:none to allow transparent toolbar control background. It fixes the problem when user changes the chart background color.

Demo screenshot or recording

No UI change in demo.

Review checklist (for reviewers only)

  • Demos all features
  • Documented/annotated
  • Matches UI/UX specs
  • Meets the code style guide
  • Accessible
  • Mobile first (responsive)
  • RTL support (bidirectional text)
  • Performant (limited bloat)

@hlyang397 hlyang397 requested review from natashadecoste, theiliad and a team as code owners November 3, 2020 07:50
@hlyang397 hlyang397 requested review from cal-smith and removed request for a team November 3, 2020 07:50
theiliad
theiliad requested changes Nov 3, 2020
View reviewed changes
Copy link
Member

@theiliad theiliad left a comment

Choose a reason for hiding this comment

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

Why would the user change the background of the chart in any of the 4 themes? That'd probably result in a lack of compliance with the design language...?

@hlyang397
Copy link
Contributor Author

hlyang397 commented Nov 4, 2020 •
edited
Loading

Why would the user change the background of the chart in any of the 4 themes? That'd probably result in a lack of compliance with the design language...?

When we set the control background to transparent (fill:none), it could work no matter user likes to follow design language or not.

Our application does use the g90 theme, but we also use a little different background color for chart area.

@hlyang397 hlyang397 requested a review from theiliad November 4, 2020 03:31
@joshkimmell
Copy link

If you don't feel comfortable allowing developers to set the background to transparent, why not give them more theme choices? In our case, the sparkline provides a visual element to our query cards, which are IDL compliant and accessible. We need the background to be transparent because of our card states, which include hover and disabled. Unless you're going to include states in the chart, transparency is the only thing that would work here.

@theiliad
Copy link
Member

theiliad commented Nov 4, 2020

If you don't feel comfortable allowing developers to set the background to transparent, why not give them more theme choices? In our case, the sparkline provides a visual element to our query cards, which are IDL compliant and accessible. We need the background to be transparent because of our card states, which include hover and disabled. Unless you're going to include states in the chart, transparency is the only thing that would work here.

I think that we definitely need to discuss this on the guild call https://www.carbondesignsystem.com/whats-happening/meetups#data-visualization-weekly-playbackwednesdays-11am12pm-central

@hlyang397
Copy link
Contributor Author

@natashadecoste @cal-smith
Could you help to review this PR ? Thanks.

cal-smith
cal-smith approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@cal-smith cal-smith left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍

@theiliad theiliad merged commit e4bf1bf into carbon-design-system:master Nov 10, 2020
@theiliad theiliad deleted the fix-toolbar-background branch November 10, 2020 15:34
@hlyang397 hlyang397 mentioned this pull request Nov 12, 2020
Merged
8 tasks
JennChao added a commit to JennChao/carbon-charts that referenced this pull request Nov 17, 2020
@JennChao
Merge branch 'master' of https://github.com/carbon-design-system/carb…
266090e 
…on-charts into sparkline-with-scale-color-option

* 'master' of https://github.com/carbon-design-system/carbon-charts: (24 commits)
  v0.41.4
  Merge pull request carbon-design-system#882 from theiliad/demo-treemap-fix
  docs(core): add theme & API tutorials to storybook (carbon-design-system#883)
  v0.41.3
  Merge pull request carbon-design-system#880 from sophiiae/fix-import-colors
  fix(core): use fill:transparent for toolbar controls (carbon-design-system#884)
  v0.41.2
  fix(core): tooltips should hide when mouseout is triggered on the chart holder (carbon-design-system#887)
  fix(core): sanitize user provided axis domains
  add treemap to the component status table
  v0.41.1
  Merge pull request carbon-design-system#719 from theiliad/tree-map
  fix(core): correct axis title paddings in y-axes (carbon-design-system#873)
  v0.41.0
  fix: use fill:none to allow transparent toolbar control background (carbon-design-system#858)
  feat(core): add alignment options for gauge charts (carbon-design-system#877)
  v0.40.14
  fix(core): replace import to lodash with lodash-es
  fix(core): only set zoombar data and sanitize date values if a cartesian time-series chart
  v0.40.13
  ...
theiliad pushed a commit to theiliad/carbon-charts that referenced this pull request May 17, 2021
@hlyang397
fix: use fill:none to allow transparent toolbar control background (c…
94e7df5 
…arbon-design-system#858)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theiliad theiliad theiliad approved these changes

@cal-smith cal-smith cal-smith approved these changes

@natashadecoste natashadecoste Awaiting requested review from natashadecoste

Assignees
No one assigned
Labels
type: design ✨
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hlyang397 @joshkimmell @theiliad @cal-smith