You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2021/03/03 19:00:09 UTC
[GitHub] [echarts] gsi7 opened a new pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
gsi7 opened a new pull request #14393:
URL: https://github.com/apache/echarts/pull/14393
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
Tooltip arrow will follow the specified `borderWidth` in tooltip.
### Fixed issues
fixes: #14373
### Are there any API changes?
- [ ] The API has been changed.
<!-- LIST THE API CHANGES HERE -->
### Related test cases or examples to use the new APIs
Checkout `test/scatter.html`
## Others
### Merging options
- [ ] Please squash the commits into a single one when merge.
### Other information
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627079561
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
Please @pissang @susiwen8 take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627085428
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
@plainheart https://developer.mozilla.org/zh-CN/docs/Web/CSS/clip-path Seems major browsers except for IE already support it well. It is acceptable having some small issue on IE to have a better experience on other major browsers IMO. Of course, it will be perfect to have other solutions both keep compatibility, simple implementation(no too much code), and good result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r663823225
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@plainheart I have addressed your comment. Thank you :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-874577445
> > Is it ready to be merged now?
>
> It looks good to me now. If other maintainers have no issue with this, it will be merged.
Alright, thanks :tada:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-880432194
LGTM. @g7i Thanks for keeping improving your PR and @plainheart thanks for the detailed review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635772986
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
Review comment:
Can have a try.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-874573213
@g7i Really thank you for the work!
@pissang Please help take a double check. I think it should be working well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-849349619
Seems we can't make it merged in 5.1.2. Moved to the next 5.2.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r634007063
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
@g7i Hi, any updates on this offset issue? We are going to finish the development of version 5.1.2 and it will be great if this PR can be included
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627858707
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
It seems both horizontal and vertical positions are off a bit. I think the position of arrow is correct(vertical middle / horizontal center of the tooltip container), but the position of the tooltip container is not correct, it should consider arrow size when transforming the container's x/y.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-827277588
@g7i Please don't commit the files in `dist` and `i18n` folder. They are only updated before release
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-860031997
@pissang @plainheart I have addressed all the comments for changes. Please re-review it. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627047660
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
The position of arrow seems not so accurated(shown as previous screenshot). We may need to minus half of the diagonal. (Not for line 77)
And the calculated number may have many decimal places, it would be better to fix it as an integer or remain two decimal places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-880434980
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627044822
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
If we add `box-shadow` for the arrow, it will look unexpectedly obvious. I'm not sure whether we should disable shadow of arrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635767092
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
Review comment:
I think so. Because it will look weird if we set a small `borderWidth`, for example, `1`.
<img src="https://user-images.githubusercontent.com/26999792/118921914-9e18d980-b96b-11eb-8ea9-45882264df88.png" width="400">
But if we make the position of the arrow correct, it will be also okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r621105925
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -111,6 +114,8 @@ function assembleTransform(x: number, y: number, toString?: boolean) {
// If using float on style, the final width of the dom might
// keep changing slightly while mouse move. So `toFixed(0)` them.
const x0 = x.toFixed(0) + 'px';
+ // To keep the tooltip slightly above
+ y -= 5;
Review comment:
What if tooltip is on the bottom/left/right?
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,28 +60,31 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ let positionStyle = `${arrowPos}:-10px;`;
Review comment:
What is this change for?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r664285310
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@g7i Thanks for your work. But I think our current calculation is still not accurate enough. It needs to be improved.
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,36 +60,45 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = Math.max(Math.round(borderWidth) * 1.5, 6);
+ let positionStyle = '';
let transformStyle = CSS_TRANSFORM_VENDOR + ':';
+ let rotateDeg = 0;
if (indexOf(['left', 'right'], arrowPos) > -1) {
positionStyle += 'top:50%';
- transformStyle += `translateY(-50%) rotate(${arrowPos === 'left' ? -225 : -45}deg)`;
+ transformStyle += `translateY(-50%) rotate(${rotateDeg = arrowPos === 'left' ? -225 : -45}deg)`;
}
else {
positionStyle += 'left:50%';
- transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
+ transformStyle += `translateX(-50%) rotate(${rotateDeg = arrowPos === 'top' ? 225 : 45}deg)`;
}
+ const arrowOffset = Math.round((borderWidth * Math.SQRT2 + arrowSize / 2) * 100) / 100;
Review comment:
```suggestion
const rotateRadian = rotateDeg * Math.PI / 180;
const arrowWH = arrowSize + borderWidth;
const rotatedWH = arrowWH * Math.abs(Math.cos(rotateRadian)) + arrowWH * Math.abs(Math.sin(rotateRadian));
const arrowOffset = Math.round(((rotatedWH - Math.SQRT2 * borderWidth) / 2
+ Math.SQRT2 * borderWidth - (rotatedWH - arrowWH) / 2) * 100) / 100;
```
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,36 +60,45 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = Math.max(Math.round(borderWidth) * 1.5, 6);
+ let positionStyle = '';
let transformStyle = CSS_TRANSFORM_VENDOR + ':';
+ let rotateDeg = 0;
Review comment:
```suggestion
let rotateDeg;
```
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,36 +60,45 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = Math.max(Math.round(borderWidth) * 1.5, 6);
+ let positionStyle = '';
let transformStyle = CSS_TRANSFORM_VENDOR + ':';
+ let rotateDeg = 0;
if (indexOf(['left', 'right'], arrowPos) > -1) {
positionStyle += 'top:50%';
- transformStyle += `translateY(-50%) rotate(${arrowPos === 'left' ? -225 : -45}deg)`;
+ transformStyle += `translateY(-50%) rotate(${rotateDeg = arrowPos === 'left' ? -225 : -45}deg)`;
}
else {
positionStyle += 'left:50%';
- transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
+ transformStyle += `translateX(-50%) rotate(${rotateDeg = arrowPos === 'top' ? 225 : 45}deg)`;
}
+ const arrowOffset = Math.round((borderWidth * Math.SQRT2 + arrowSize / 2) * 100) / 100;
+ positionStyle += `;${arrowPos}:-${arrowOffset}px`;
+ const margin = Math.round(borderWidth / 2 * Math.abs(Math.cos(rotateDeg)) * 100) / 100;
Review comment:
```suggestion
```
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@g7i You could use various `borderWidth` to test if the arrow will be off a bit.
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,36 +60,45 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = Math.max(Math.round(borderWidth) * 1.5, 6);
+ let positionStyle = '';
let transformStyle = CSS_TRANSFORM_VENDOR + ':';
+ let rotateDeg;
if (indexOf(['left', 'right'], arrowPos) > -1) {
positionStyle += 'top:50%';
- transformStyle += `translateY(-50%) rotate(${arrowPos === 'left' ? -225 : -45}deg)`;
+ transformStyle += `translateY(-50%) rotate(${rotateDeg = arrowPos === 'left' ? -225 : -45}deg)`;
}
else {
positionStyle += 'left:50%';
- transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
+ transformStyle += `translateX(-50%) rotate(${rotateDeg = arrowPos === 'top' ? 225 : 45}deg)`;
}
+ const arrowOffset = Math.round((borderWidth * Math.SQRT2 + arrowSize / 2) * 100) / 100;
+ positionStyle += `;${arrowPos}:-${arrowOffset}px`;
+ const margin = Math.round(borderWidth / 2 * Math.abs(Math.cos(rotateDeg)) * 100) / 100;
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
+ `margin-${arrowPos}:${margin}px;`,
Review comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627085428
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
@plainheart https://developer.mozilla.org/zh-CN/docs/Web/CSS/clip-path Major browsers except for IE already support it well. It is acceptable having some small issue on IE to have a better experience on other major browsers IMO. Of course, it will be perfect to have other solutions both keep compatibility, simple implementation(no too much code), and good result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627017608
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
Review comment:
Fixed :exclamation:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r664285310
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@g7i Thanks for your work. But I think our current calculation is still not accurate enough. It needs to be improved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635387439
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
Review comment:
It seems to be not necessary to limit border width?
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -1100,20 +1101,20 @@ function calcTooltipPosition(
y = rect.y + rectHeight / 2 - domHeight / 2;
break;
case 'top':
- x = rect.x + rectWidth / 2 - domWidth / 2;
+ x = rect.x + rectWidth / 2 - domWidth / 2 - borderWidth / 2;
Review comment:
I just found the offset is caused by the `getSize` function.
https://github.com/apache/echarts/blob/2b39cb408712ce3710801dd5219d0a14aad5b3ce/src/component/tooltip/TooltipHTMLContent.ts#L441-L444
We can see it's currently using `clientWidth` and `clientHeight`, which doesn't include the border width.
So it would be fixed easily if we use `offsetWidth` and `offsetHeight`. And then it would be unnecessary to subtract border width here.
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -1084,7 +1084,8 @@ function confineTooltipPosition(
function calcTooltipPosition(
position: TooltipOption['position'],
rect: ZRRectLike,
- contentSize: number[]
+ contentSize: number[],
+ borderWidth: number
): [number, number] {
Review comment:
By the review comment in https://github.com/apache/echarts/blob/2b39cb408712ce3710801dd5219d0a14aad5b3ce/src/component/tooltip/TooltipView.ts#L1104
the tooltip may be a bit far away from the element.
Let us take a look at the following lines
https://github.com/apache/echarts/blob/2b39cb408712ce3710801dd5219d0a14aad5b3ce/src/component/tooltip/TooltipView.ts#L1092-L1093
The usage of `offset` and `gap` looks similar and their value is a fixed number. May we merge them into one value calculated with the border width?
For example,
```js
// Sure, if `borderWidth` is very small or 0, we could also set a limit of minimized value.
Math.sqrt(2 * borderWidth * borderWidth)
```
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
Still here. I think it would be better to retain 1 decimal place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627076808
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
So, should I remove the shadow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635327145
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
Good job! We'll help review it ASAP. 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635325603
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
@pissang updated the PR. :smiley:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r663622792
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
It may be not appropriate to use `borderWidth` as arrow size, but perhaps we can multiply it by a factor. And the calculated `arrowOffset` seems to be not accurate enough. Here is my humble opinion about the calculation,
```diff
src/component/tooltip/TooltipHTMLContent.ts
@@ -73,22 +73,27 @@ function assembleArrow(
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- const arrowOffset = Math.round(Math.sqrt(2 * borderWidth * borderWidth) / 2) + borderWidth;
- let positionStyle = `${arrowPos}:-${arrowOffset}px;`;
+ const arrowSize = Math.max(Math.round(borderWidth) * 1.5, 6);
+ let positionStyle = '';
let transformStyle = CSS_TRANSFORM_VENDOR + ':';
+ let rotateDeg = 0;
if (indexOf(['left', 'right'], arrowPos) > -1) {
positionStyle += 'top:50%';
- transformStyle += `translateY(-50%) rotate(${arrowPos === 'left' ? -225 : -45}deg)`;
+ transformStyle += `translateY(-50%) rotate(${rotateDeg = arrowPos === 'left' ? -225 : -45}deg)`;
}
else {
positionStyle += 'left:50%';
- transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
+ transformStyle += `translateX(-50%) rotate(${rotateDeg = arrowPos === 'top' ? 225 : 45}deg)`;
}
+ const arrowOffset = Math.round((borderWidth * Math.SQRT2 + arrowSize / 2) * 100) / 100;
+ positionStyle += `;${arrowPos}:-${arrowOffset}px`;
+ const margin = Math.round(borderWidth / 2 * Math.abs(Math.cos(rotateDeg)) * 100) / 100;
const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
+ `margin-${arrowPos}:${margin}px;`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
`background-color:${backgroundColor};`
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-874573213
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r624428061
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -111,6 +114,8 @@ function assembleTransform(x: number, y: number, toString?: boolean) {
// If using float on style, the final width of the dom might
// keep changing slightly while mouse move. So `toFixed(0)` them.
const x0 = x.toFixed(0) + 'px';
+ // To keep the tooltip slightly above
+ y -= 5;
Review comment:
Fixed automatically when I adjusted the calculations for the comment below. So removed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627373271
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
@g7i I don't think it's possible to use pseudo-elements in inline style. We can apply the `clip-path` on our arrow element.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang merged pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #14393:
URL: https://github.com/apache/echarts/pull/14393
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627080222
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
I remember we dicussed it before and I prefereed not having shadow here. I'm not sure if there is a better solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635762506
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
Review comment:
I think we're not modifying `borderWidth`, just the `size of the arrow` if borderWidth is less than 5 because below this the arrow is very very small and looking weird.
Do you think I should remove it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627047660
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
The position of arrow seems not so accurated(shown as previous screenshot). We may need to minus half of the diagonal. (Not for line 77) Perhaps, we should apply the offset to `transform` of the tooltip container rather than the arrow self.
And the calculated number may have many decimal places, it would be better to fix it as an integer or remain two decimal places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635389779
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
Still here. I think it would be better to retain 1 decimal place.
Or no decimal place? This is comparison between `-5.5px` and `-5px`
**-5.5px**
<img src="https://user-images.githubusercontent.com/26999792/118922847-467b6d80-b96d-11eb-97b4-e0b6ef518e92.png" width="400">
**-5px**
<img src="https://user-images.githubusercontent.com/26999792/118922827-3bc0d880-b96d-11eb-8cb2-640f6ae2bc14.png" width="400">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627083203
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
`clip-path` should work. However, it seems to have poor compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-826833511
@g7i Hi, It will be great if you can resolve the conflicts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627343082
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
One quick question, how am I supposed to use pseudo-elements in inline style? Is there any way or linked stylesheet so that I can add them there?
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r622089871
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,28 +60,31 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ let positionStyle = `${arrowPos}:-10px;`;
Review comment:
Yeah, on it.
Since the square (which makes the arrow) is rotated need to calculate the diagonal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r664344993
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,36 +60,45 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = Math.max(Math.round(borderWidth) * 1.5, 6);
+ let positionStyle = '';
let transformStyle = CSS_TRANSFORM_VENDOR + ':';
+ let rotateDeg = 0;
if (indexOf(['left', 'right'], arrowPos) > -1) {
positionStyle += 'top:50%';
- transformStyle += `translateY(-50%) rotate(${arrowPos === 'left' ? -225 : -45}deg)`;
+ transformStyle += `translateY(-50%) rotate(${rotateDeg = arrowPos === 'left' ? -225 : -45}deg)`;
}
else {
positionStyle += 'left:50%';
- transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
+ transformStyle += `translateX(-50%) rotate(${rotateDeg = arrowPos === 'top' ? 225 : 45}deg)`;
}
+ const arrowOffset = Math.round((borderWidth * Math.SQRT2 + arrowSize / 2) * 100) / 100;
Review comment:
```suggestion
const rotateRadian = rotateDeg * Math.PI / 180;
const arrowWH = arrowSize + borderWidth;
const rotatedWH = arrowWH * Math.abs(Math.cos(rotateRadian)) + arrowWH * Math.abs(Math.sin(rotateRadian));
const arrowOffset = Math.round(((rotatedWH - Math.SQRT2 * borderWidth) / 2
+ Math.SQRT2 * borderWidth - (rotatedWH - arrowWH) / 2) * 100) / 100;
```
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,36 +60,45 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = Math.max(Math.round(borderWidth) * 1.5, 6);
+ let positionStyle = '';
let transformStyle = CSS_TRANSFORM_VENDOR + ':';
+ let rotateDeg = 0;
Review comment:
```suggestion
let rotateDeg;
```
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,36 +60,45 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = Math.max(Math.round(borderWidth) * 1.5, 6);
+ let positionStyle = '';
let transformStyle = CSS_TRANSFORM_VENDOR + ':';
+ let rotateDeg = 0;
if (indexOf(['left', 'right'], arrowPos) > -1) {
positionStyle += 'top:50%';
- transformStyle += `translateY(-50%) rotate(${arrowPos === 'left' ? -225 : -45}deg)`;
+ transformStyle += `translateY(-50%) rotate(${rotateDeg = arrowPos === 'left' ? -225 : -45}deg)`;
}
else {
positionStyle += 'left:50%';
- transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
+ transformStyle += `translateX(-50%) rotate(${rotateDeg = arrowPos === 'top' ? 225 : 45}deg)`;
}
+ const arrowOffset = Math.round((borderWidth * Math.SQRT2 + arrowSize / 2) * 100) / 100;
+ positionStyle += `;${arrowPos}:-${arrowOffset}px`;
+ const margin = Math.round(borderWidth / 2 * Math.abs(Math.cos(rotateDeg)) * 100) / 100;
Review comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627085428
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
@plainheart https://developer.mozilla.org/zh-CN/docs/Web/CSS/clip-path Seems major browsers except for IE already support it well. It is acceptable having some small issue on IE to have a better experience on other major browsers IMO. Of course, it will be perfect to have other solutions that both keep compatibility, simple implementation(no too much code), and good result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-826638625
Hey, do I need to resolve the conflicts or the maintainer will resolve?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i removed a comment on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i removed a comment on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-874574160
Is it ready to be merged now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r664350834
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,36 +60,45 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = Math.max(Math.round(borderWidth) * 1.5, 6);
+ let positionStyle = '';
let transformStyle = CSS_TRANSFORM_VENDOR + ':';
+ let rotateDeg;
if (indexOf(['left', 'right'], arrowPos) > -1) {
positionStyle += 'top:50%';
- transformStyle += `translateY(-50%) rotate(${arrowPos === 'left' ? -225 : -45}deg)`;
+ transformStyle += `translateY(-50%) rotate(${rotateDeg = arrowPos === 'left' ? -225 : -45}deg)`;
}
else {
positionStyle += 'left:50%';
- transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
+ transformStyle += `translateX(-50%) rotate(${rotateDeg = arrowPos === 'top' ? 225 : 45}deg)`;
}
+ const arrowOffset = Math.round((borderWidth * Math.SQRT2 + arrowSize / 2) * 100) / 100;
+ positionStyle += `;${arrowPos}:-${arrowOffset}px`;
+ const margin = Math.round(borderWidth / 2 * Math.abs(Math.cos(rotateDeg)) * 100) / 100;
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
+ `margin-${arrowPos}:${margin}px;`,
Review comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r634007063
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
@g7i Hi, any updates on this offset issue? We are going to finish the development of version 5.1.2 and hope this PR can be included
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r621116697
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,28 +60,31 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ let positionStyle = `${arrowPos}:-10px;`;
Review comment:
When I increased the `borderWidth` of the arrow, the placement is slightly off. So in order to adjust it I made this change.
If there is any better way please suggest me.
Thanks....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r634160118
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
@g7i Great!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627360185
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
You mean horizontal placement right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-827282827
> @g7i Please don't commit the files in `dist` and `i18n` folder. They are only updated before release
Ohh okay, sorry did it by mistake. Now reverted the the changes. Thanks....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-874575867
> Is it ready to be merged now?
It looks good to me now. If other maintainers have no issue with this, it will be merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r629806872
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
@pissang Apparently we can't have `box-shadow` on a clipped shape. Another workaround I tried is by having two nested `div`, clipping inner and applying `filter: drop-shadow()` to outer element but the result was not satisfying. We'll have shadow above as well.
Is there any another way or should I remove `box-shadow` from the arrow :question:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627858707
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
It seems both horizontal and vertical positions are off a bit. I think the position of arrow is correct(vertical middle / horizontal center of the tooltip container), but the position of the tooltip container is not correct, it should consider arrow size when transforming the container's x/y.
What expected is like this
<img src="https://user-images.githubusercontent.com/26999792/117383828-38acfd80-af14-11eb-9ef1-7e3eae615d21.png" width="200">
However, now it is
<img src="https://user-images.githubusercontent.com/26999792/117383929-6db95000-af14-11eb-9bfb-55da54d2bdc9.png" width="200">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635403180
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -1084,7 +1084,8 @@ function confineTooltipPosition(
function calcTooltipPosition(
position: TooltipOption['position'],
rect: ZRRectLike,
- contentSize: number[]
+ contentSize: number[],
+ borderWidth: number
): [number, number] {
Review comment:
By the [review comment](https://github.com/apache/echarts/pull/14393#discussion_r635372765) in https://github.com/apache/echarts/blob/2b39cb408712ce3710801dd5219d0a14aad5b3ce/src/component/tooltip/TooltipView.ts#L1104
the tooltip may be a bit far away from the element.
Let us take a look at the following lines
https://github.com/apache/echarts/blob/2b39cb408712ce3710801dd5219d0a14aad5b3ce/src/component/tooltip/TooltipView.ts#L1092-L1093
The usage of `offset` and `gap` looks similar and their value is a fixed number. May we merge them into one value calculated with the border width?
For example,
```js
// Sure, if `borderWidth` is very small or 0, we could also set a limit of minimized value.
Math.sqrt(2 * borderWidth * borderWidth)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-874571551
Thanks for helping me out :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r663819930
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
Sure, these calculations seems more accurate. Thanks :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-874571551
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r650372816
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -1100,20 +1101,20 @@ function calcTooltipPosition(
y = rect.y + rectHeight / 2 - domHeight / 2;
break;
case 'top':
- x = rect.x + rectWidth / 2 - domWidth / 2;
+ x = rect.x + rectWidth / 2 - domWidth / 2 - borderWidth / 2;
Review comment:
:heavy_check_mark:
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
Review comment:
:heavy_check_mark:
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
:heavy_check_mark:
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -1084,7 +1084,8 @@ function confineTooltipPosition(
function calcTooltipPosition(
position: TooltipOption['position'],
rect: ZRRectLike,
- contentSize: number[]
+ contentSize: number[],
+ borderWidth: number
): [number, number] {
Review comment:
:heavy_check_mark:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627044822
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
If we add `box-shadow` for the arrow, it will look unexpectedly obvious. I'm not sure whether we should disable shadow of arrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635767092
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
Review comment:
I think so. Because it will look weird if we set a small `borderWidth`, for example, `1`.
<img src="https://user-images.githubusercontent.com/26999792/118921914-9e18d980-b96b-11eb-8ea9-45882264df88.png" width="400">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r634115148
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
I'm almost done with it. Will update the PR by tomorrow EOD.
Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627373271
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
@g7i I don't think it's possible to use pseudo-elements in inline style. So the top solution in my StackOverflow link can't be used in our case. Instead the second solution which uses `clip-path` is possible here. We can apply `clip-path` on our arrow element.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r635771348
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
Review comment:
So, for smaller borderwidth I'll handle position in an `if` condition. Is this fine?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627373271
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
@g7i I don't think it's possible to use pseudo-elements in inline style. But we don't need it here. We can apply the `clip-path` on our arrow element.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r661098568
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
I'm not sure but is it correct to use `borderWidth` here? @plainheart
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627080659
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
Perhaps CSS `clip-path` like described in https://stackoverflow.com/questions/43863785/css-arrow-with-border-add-box-shadow/43863886 can fix it perfectly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r664342104
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@plainheart I have replaced everything you suggested in the comment. Could you please tell me the cases where the calculation is getting off so that I can improve. Thank you...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-789978308
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/echarts/wiki/How-to-make-a-pull-request).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627080659
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
Perhaps we need CSS `clip-path` like described in https://stackoverflow.com/questions/43863785/css-arrow-with-border-add-box-shadow/43863886
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-874574160
Is it ready to be merged now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r624428141
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,28 +60,31 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ let positionStyle = `${arrowPos}:-10px;`;
Review comment:
Fixed it :exclamation:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang edited a comment on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-849349619
Seems we can't make it merged in 5.1.2. Let's continue it in the next 5.2.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r622089871
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,28 +60,31 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ let positionStyle = `${arrowPos}:-10px;`;
Review comment:
Yeah, on it.
Since the square of the arrow is rotated need to calculate the diagonal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] susiwen8 commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627008124
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
Review comment:
Just notice a very small thing, could we move this after `if` statement? Since it only affect whatever come after `if`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] susiwen8 commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627008124
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
Review comment:
Just notice a very small thing, could it move this after `if` statement? Since it only affect whatever come after `if`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r663823225
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@plainheart I have addressed your comment. Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r664348567
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@g7i You could use various `borderWidth` to test if the arrow will be off a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627047660
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
The position of arrow seems not so accurated(shown as previous screenshot). We may need to minus half of the border width. (Not for line 77)
And the calculated number may have many decimal places, it would be better to fix it as an integer or remain two decimal places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627858707
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
It seems both horizontal and vertical positions are off a bit. I think the position of arrow is correct(vertical middle / horizontal center of the tooltip container), but the position of the tooltip container is not correct, it should consider arrow size when transforming the container's x/y.
What expected is like this
<img src="https://user-images.githubusercontent.com/26999792/117383828-38acfd80-af14-11eb-9ef1-7e3eae615d21.png" width="200">
<img src="https://user-images.githubusercontent.com/26999792/117384509-b9b8c480-af15-11eb-8df4-e67736d8aaf9.png" width="200">
However, now it is
<img src="https://user-images.githubusercontent.com/26999792/117383929-6db95000-af14-11eb-9bfb-55da54d2bdc9.png" width="200">
<img src="https://user-images.githubusercontent.com/26999792/117384260-27b0bc00-af15-11eb-9194-ee8819e57864.png" width="200">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i edited a comment on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i edited a comment on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-827282827
> @g7i Please don't commit the files in `dist` and `i18n` folder. They are only updated before release
Ohh okay, sorry did it by mistake. Now reverted the changes. Thanks....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r621765386
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,28 +60,31 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ let positionStyle = `${arrowPos}:-10px;`;
Review comment:
If it is related to user-set `borderWidth`. I think it's better to calculate this value from `broderWidth`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r661098568
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
I'm not sure but is it correct to use borderWidth here? @plainheart
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r663819930
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
Sure, these calculations seems more accurate. Thanks :)
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@plainheart I have addressed your comment. Thank you :)
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@plainheart I have addressed your comment. Thank you
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,15 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${borderWidth}px;height:${borderWidth}px;`,
Review comment:
@plainheart I have replaced everything you suggested in the comment. Could you please tell me the cases where the calculation is getting off so that I can improve. Thank you...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r634007063
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
@g7i Hi, any updates on this offset issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627047660
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
The position of arrow seems not so accurated(shown as previous screenshot). We may need to minus half of the border width.
And the calculated number may have many decimal places, it would be better to fix it as an integer or remain two decimal places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627044822
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
If we add `box-shadow` for the arrow, it will look unexpectedly obvious. I'm not sure whether we should disable shadow of arrow.
<img src="https://user-images.githubusercontent.com/26999792/117236026-73eaf600-ae5a-11eb-968d-42b6c8b9c155.png" width="400">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r629813496
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -81,15 +85,23 @@ function assembleArrow(
transformStyle += `translateX(-50%) rotate(${arrowPos === 'top' ? 225 : 45}deg)`;
}
- const borderStyle = `${borderColor} solid 1px;`;
+ const borderStyle = `${borderColor} solid ${borderWidth}px;`;
const styleCss = [
- 'position:absolute;width:10px;height:10px;',
+ `position:absolute;width:${arrowSize}px;height:${arrowSize}px;`,
`${positionStyle};${transformStyle};`,
`border-bottom:${borderStyle}`,
`border-right:${borderStyle}`,
- `background-color:${backgroundColor};`,
- 'box-shadow:8px 8px 16px -3px #000;'
+ `background-color:${backgroundColor};`
];
+
+ const shadowBlur = tooltipModel.get('shadowBlur');
+ const shadowColor = tooltipModel.get('shadowColor');
+ const shadowOffsetX = tooltipModel.get('shadowOffsetX');
+ const shadowOffsetY = tooltipModel.get('shadowOffsetY');
+ const boxShadow = `${shadowOffsetX}px ${shadowOffsetY}px ${shadowBlur}px ${shadowColor}`;
+
+ styleCss.push('box-shadow:' + boxShadow);
Review comment:
@g7i You can remove `box-shadow` in this PR. We can figure the shadow out later.
But I'm not sure why we can't have `box-shadow` on a clipped shape. It seems works well in the above link.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-850995176
Okay, thanks. I was away for a while that's why unable to work on it. Will fix it asap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] susiwen8 commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r627008124
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
Review comment:
Just notice a very small thing, could it move this after `if` statement? Since it only affect whatever after `if`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r621117096
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -111,6 +114,8 @@ function assembleTransform(x: number, y: number, toString?: boolean) {
// If using float on style, the final width of the dom might
// keep changing slightly while mouse move. So `toFixed(0)` them.
const x0 = x.toFixed(0) + 'px';
+ // To keep the tooltip slightly above
+ y -= 5;
Review comment:
Ahh I didn't consider it. Let me rework on it. Thanks...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i commented on a change in pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i commented on a change in pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#discussion_r634113769
##########
File path: src/component/tooltip/TooltipHTMLContent.ts
##########
@@ -60,17 +60,21 @@ function mirrorPos(pos: string): string {
}
function assembleArrow(
- backgroundColor: ColorString,
+ tooltipModel: Model<TooltipOption>,
borderColor: ZRColor,
arrowPosition: TooltipOption['position']
) {
if (!isString(arrowPosition) || arrowPosition === 'inside') {
return '';
}
+ const backgroundColor = tooltipModel.get('backgroundColor');
+ const borderWidth = tooltipModel.get('borderWidth');
+
borderColor = convertToColorString(borderColor);
const arrowPos = mirrorPos(arrowPosition);
- let positionStyle = `${arrowPos}:-6px;`;
+ const arrowSize = borderWidth >= 5 ? borderWidth : 5;
+ let positionStyle = `${arrowPos}:-${Math.sqrt(2 * arrowSize * arrowSize) / 2 + borderWidth}px;`;
Review comment:
I'm almost done with it. By tomorrow EOD will update the PR.
Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] g7i removed a comment on pull request #14393: bug(tooltip): tooltip arrow will follow borderWidth. close #14373
Posted by GitBox <gi...@apache.org>.
g7i removed a comment on pull request #14393:
URL: https://github.com/apache/echarts/pull/14393#issuecomment-874574160
Is it ready to be merged now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org