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(tooltip.textStyle): fix color not working #13848

Merged
merged 6 commits into from Jan 8, 2021

Conversation

susiwen8
Copy link
Contributor

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Fix tooltip.textStyle.color not working

Fixed issues

Close #13797

Details

Before: What was the problem?

Screen Shot 2020-12-20 at 17 20 15

After: How is it fixed in this PR?

Screen Shot 2020-12-20 at 17 20 52

Usage

Are there any API changes?

  • The API has been changed.

Related test cases or examples to use the new APIs

NA.

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

tooltip name and value can be configured separately. More textStyle support

@echarts-bot
Copy link

echarts-bot bot commented Dec 20, 2020

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

The pull request is marked to be PR: author is committer because you are a committer of this project.

): string {
const styles: Dictionary<unknown>[] = [TOOLTIP_VALUE_TEXT_STYLE_RICH];
const styles: Dictionary<unknown>[] = [style];
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of unknown, use RichTextStyle? i.e.:

const styles: Dictionary[] = [style];

Copy link
Contributor

Choose a reason for hiding this comment

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

@chfw I noticed @100pah is using Dictionary<unknown> to represent the style object in this module. I think it's mainly because the style can be both RichTextStyle or normal CSS style for HTML Text.

Copy link
Member

@100pah 100pah Jan 4, 2021

Choose a reason for hiding this comment

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

In fact, the accurate type of ctx.markupStyleCreator.richTextStyles should be Dictionary<TextStylePropsPart>, which is the same as textStyle.rich.
I think it's OK to either leave it at that or change it.

Comment on lines 323 to 339
return renderMode === 'richText'
? (
(noMarker ? '' : markerStr)
+ (noName ? '' : wrapInlineNameRichText(ctx, readableName))
+ (noName ? '' : wrapInlineNameRichText(ctx, readableName, nameStyle as RichTextStyle))
// Value has commas inside, so use ' ' as delimiter for multiple values.
+ (noValue ? '' : wrapInlineValueRichText(
ctx, readableValueList, valueAlignRight, valueCloseToMarker
ctx, readableValueList, valueAlignRight, valueCloseToMarker, valueStyle as RichTextStyle
))
)
: wrapBlockHTML(
(noMarker ? '' : markerStr)
+ (noName ? '' : wrapInlineNameHTML(readableName, !noMarker))
+ (noName ? '' : wrapInlineNameHTML(readableName, !noMarker, nameStyle as string))
+ (noValue ? '' : wrapInlineValueHTML(
readableValueList, valueAlignRight, valueCloseToMarker
readableValueList, valueAlignRight, valueCloseToMarker, valueStyle as string
)),
topMarginForOuterGap
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here is an abuse of short hand if-else statement. It is inviting bugs to live inside and adds difficulty in debugging it.

src/component/tooltip/tooltipMarkup.ts Outdated Show resolved Hide resolved
src/component/tooltip/tooltipMarkup.ts Outdated Show resolved Hide resolved
src/component/tooltip/tooltipMarkup.ts Show resolved Hide resolved
): string {
const styles: Dictionary<unknown>[] = [TOOLTIP_VALUE_TEXT_STYLE_RICH];
const styles: Dictionary<unknown>[] = [style];
Copy link
Contributor

Choose a reason for hiding this comment

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

@chfw I noticed @100pah is using Dictionary<unknown> to represent the style object in this module. I think it's mainly because the style can be both RichTextStyle or normal CSS style for HTML Text.

@pissang
Copy link
Contributor

pissang commented Jan 4, 2021

Sorry for the late review. I left my comments about some tiny change suggestions. Other changes look good to me. @100pah can have a double check.

PS: I just updated this branch to the latest code from master.

valueStyle: TextStyle
} {
const nameFontColor = textStyle.color || '#6e7079';
const nameFontSize = textStyle.fontSize || '12px';
Copy link
Member

Choose a reason for hiding this comment

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

By convention, fontSize in user option should be 12 rather than 12px.

Copy link
Contributor Author

@susiwen8 susiwen8 Jan 4, 2021

Choose a reason for hiding this comment

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

@100pah I don't get it. If fontSize default value were 12, then it would become font-size:12, which is not the correct format.

Copy link
Member

Choose a reason for hiding this comment

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

@susiwen8
I means that user will set fontSize in option like

tooltip: {
    textStyle: { fontSize: 14 }
}

And then textStyle.fontSize has the value 14 rather than 14px here.
And then the html template should be like:

return {
    nameStyle: `font-size:${nameFontSize}px;color:${nameFontColor};font-weight:${nameFontWeight}`,
    valueStyle: `font-size:${valueFontSize}px;color:${valueFontColor};font-weight:${valueFontWeight}`
}

If we intend to support other unit like em/rem/... in future, it would be another topic, and could be discussed in new thread.

const nameFontColor = textStyle.color || '#6e7079';
const nameFontSize = textStyle.fontSize || '12px';
const valueFontColor = textStyle.color || '#464646';
const valueFontSize = textStyle.fontSize || '14px';
Copy link
Member

Choose a reason for hiding this comment

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

14 rather than 14px.

): string {
const styles: Dictionary<unknown>[] = [TOOLTIP_VALUE_TEXT_STYLE_RICH];
const styles: Dictionary<unknown>[] = [style];
Copy link
Member

@100pah 100pah Jan 4, 2021

Choose a reason for hiding this comment

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

In fact, the accurate type of ctx.markupStyleCreator.richTextStyles should be Dictionary<TextStylePropsPart>, which is the same as textStyle.rich.
I think it's OK to either leave it at that or change it.

valueStyle: TextStyle
} {
const nameFontColor = textStyle.color || '#6e7079';
const nameFontSize = textStyle.fontSize || '12px';
Copy link
Member

Choose a reason for hiding this comment

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

@susiwen8
I means that user will set fontSize in option like

tooltip: {
    textStyle: { fontSize: 14 }
}

And then textStyle.fontSize has the value 14 rather than 14px here.
And then the html template should be like:

return {
    nameStyle: `font-size:${nameFontSize}px;color:${nameFontColor};font-weight:${nameFontWeight}`,
    valueStyle: `font-size:${valueFontSize}px;color:${valueFontColor};font-weight:${valueFontWeight}`
}

If we intend to support other unit like em/rem/... in future, it would be another topic, and could be discussed in new thread.

@susiwen8
Copy link
Contributor Author

susiwen8 commented Jan 6, 2021

@100pah @100pah Hi, I have correct my code, please review it again, thanks

@susiwen8 susiwen8 requested a review from 100pah January 6, 2021 04:37
@pissang
Copy link
Contributor

pissang commented Jan 8, 2021

LGTM now:)

@100pah 100pah merged commit 68b7fab into apache:master Jan 8, 2021
@echarts-bot
Copy link

echarts-bot bot commented Jan 8, 2021

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@susiwen8 susiwen8 deleted the tooltip-textStyle branch January 8, 2021 04:19
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.0 pie下 tooltip 的textStyle.color 设置无效
4 participants