[GitHub] [echarts] 100pah commented on a diff in pull request #16825: Changed calculated label gap to be passed from top

[GitBox <gi...@apache.org>]

100pah commented on code in PR #16825:
URL: https://github.com/apache/echarts/pull/16825#discussion_r954050188


##########
src/component/axis/AxisBuilder.ts:
##########
@@ -602,9 +622,12 @@ function isTwoLabelOverlapped(
 }
 
 function isNameLocationCenter(nameLocation: string) {
-    return nameLocation === 'middle' || nameLocation === 'center';
+    return nameLocation === 'middle' || nameLocation === 'center' || nameLocation === 'outsideMiddle';
 }
 
+function isNameLocationOutside(nameLocation: string) {
+    return nameLocation.toLowerCase().includes('outside');

Review Comment:
   echarts has no polyfills for those methods yet. 
   The compatibility of `includes` seems to be not good enough.
   Can be`/[oO]utside/.test(nameLocation)`.



##########
src/component/axis/AxisBuilder.ts:
##########
@@ -477,15 +496,16 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
 
 };
 
-function endTextLayout(
-    rotation: number, textPosition: 'start' | 'middle' | 'end', textRotate: number, extent: number[]
-) {
+function endTextLayout(rotation: number,
+    textPosition: 'start' | 'middle' | 'end' | 'outsideStart' | 'outsideMiddle' | 'outsideEnd',
+    textRotate: number, extent: number[]) {
     const rotationDiff = remRadian(textRotate - rotation);
     let textAlign: ZRTextAlign;
     let textVerticalAlign: ZRTextVerticalAlign;
     const inverse = extent[0] > extent[1];
-    const onLeft = (textPosition === 'start' && !inverse)
-        || (textPosition !== 'start' && inverse);
+    const textIsStart = textPosition.toLocaleLowerCase().includes('start');

Review Comment:
   echarts has no polyfills for those methods yet. 
   The compatibility of `includes` seems to be not good enough.
   Can be`/[sS]tart/.test(textPosition)`.



##########
src/component/axis/AxisBuilder.ts:
##########
@@ -352,7 +353,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
         buildAxisMinorTicks(group, transformGroup, axisModel, opt.tickDirection);
 
         // This bit fixes the label overlap issue for the time chart.
-        // See https://github.com/apache/echarts/issues/14266 for more.
+        // See https://github.com/apache/echarts/issues/142156 for more.

Review Comment:
   142156 is not correct.



##########
src/component/axis/AxisBuilder.ts:
##########
@@ -367,29 +368,47 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
     },
 
     axisName(opt, axisModel, group, transformGroup) {
+        function calcDistanceToAxis() {
+            const defaultMargin = 10;
+            const axis = axisModel.axis;
+            // const isHorizontal = axis.getRotate() === 0 || axis.getRotate() === 180;
+            const isHorizontal = true;
+            const labelUnionRect = estimateLabelUnionRect(axis);
+            if (!labelUnionRect) {
+                return 0;
+            }
+            const dim = isHorizontal ? 'height' : 'width';
+            const margin = axisModel.getModel('axisLabel').get('margin');
+            return labelUnionRect[dim] + margin + defaultMargin;
+        }
         const name = retrieve(opt.axisName, axisModel.get('name'));
 
         if (!name) {
             return;
         }
-
+        const labelGap = calcDistanceToAxis();

Review Comment:
   Should better call `calcDistanceToAxis` only when `labelGap` is required, that is, when `nameLocation` is `'outsideXxx'`. Because `calcDistanceToAxis` calls `estimateLabelUnionRect`, which is not a lite operation and may not work right in some case (like in `polar`)



##########
src/component/axis/AxisBuilder.ts:
##########
@@ -367,29 +368,47 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
     },
 
     axisName(opt, axisModel, group, transformGroup) {
+        function calcDistanceToAxis() {
+            const defaultMargin = 10;
+            const axis = axisModel.axis;
+            // const isHorizontal = axis.getRotate() === 0 || axis.getRotate() === 180;
+            const isHorizontal = true;

Review Comment:
   Use `axis.isHorizontal()`



-- 
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