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(core): title string contains html ascii characters #547

Merged
merged 6 commits into from
Apr 9, 2020

Conversation

natashadecoste
Copy link
Contributor

@natashadecoste natashadecoste commented Mar 30, 2020

Title strings passed into the options are HTML number encoded (apostrophes, etc -> ')
Setting to .text() renders the string as is, setting to .html() renders the string after it is decoded. The DOM html is unaffected by the change.

This bug appears in the legend (see below) and axis titles but not within the toolltip (toolltip passes the strings through .html())

image

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)

@netlify
Copy link

netlify bot commented Mar 30, 2020

Deploy preview for carbon-charts-react ready!

Built with commit 3007a84

https://deploy-preview-547--carbon-charts-react.netlify.com

@netlify
Copy link

netlify bot commented Mar 30, 2020

Deploy preview for carbon-charts-vue ready!

Built with commit 3007a84

https://deploy-preview-547--carbon-charts-vue.netlify.com

@netlify
Copy link

netlify bot commented Mar 30, 2020

Deploy preview for carbon-charts-angular ready!

Built with commit 3007a84

https://deploy-preview-547--carbon-charts-angular.netlify.com

.append("tspan")
.text("...");

// add events for displaying the tooltip with the title
const self = this;
title
.on("mouseenter", function() {
.on("mouseenter", function () {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.on("mouseenter", function () {
.on("mouseenter", function() {

self.services.events.dispatchEvent("show-tooltip", {
hoveredElement: title,
type: TooltipTypes.TITLE
});
})
.on("mouseout", function() {
.on("mouseout", function () {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.on("mouseout", function () {
.on("mouseout", function() {

@natashadecoste natashadecoste marked this pull request as ready for review April 7, 2020 20:18
@natashadecoste natashadecoste requested a review from a team as a code owner April 7, 2020 20:18
zvonimirfras
zvonimirfras previously approved these changes Apr 8, 2020
@@ -22,7 +22,7 @@ export const lineData = [
];

export const lineOptions = {
title: "Line (discrete)",
title: "Line (discrete) - long long long long long long long long long long long long long long time format ('MMM d, hh a and 'hh a')",
Copy link
Member

Choose a reason for hiding this comment

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

we should just keep this short like before. as long as it works we're good

@theiliad theiliad merged commit 1a6b646 into carbon-design-system:master Apr 9, 2020
@theiliad theiliad added the type: bug 🐛 Something isn't working label Apr 9, 2020
theiliad pushed a commit to theiliad/carbon-charts that referenced this pull request May 17, 2021
…-system#547)

* fix(core): title string contains html ascii characters for certain symbols

* update for legend and axis text

* fix demo date
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants