added prop rotateAngle to avoid the overlapping x-axis tick values #322

udaymattam · 2023-11-06T10:54:00Z

Summary

What was changed:
added new prop called rotateAngle to avoid the x-axis overlapping tick values.
with prop rotateAngle: -45

with prop rotateAngle: 40

Why it was changed:

Testing

This change was tested using:

Reviews

In addition to engineering reviews, this PR needs:

UX review
Accessibility review
Functional review

Additional Details

This PR resolves:

UXPLATFORM-XXXX

Thank you for contributing to Terra.
@cerner/terra

sdadn · 2023-11-06T16:43:06Z

packages/carbon-graphs/CHANGELOG.md

+* Fixed
+  * Fixed tick values overlapping on X-axis by rotating 15,30,45.


Suggested change

* Fixed

* Fixed tick values overlapping on X-axis by rotating 15,30,45.

* Added

* Added prop `rotateAngle` to allow rotation of x-axis tick labels to prevent overlap.

resolved in 3a9d13e

sdadn · 2023-11-06T16:56:14Z

packages/carbon-graphs/src/js/helpers/axis.js

@@ -881,6 +881,8 @@ const getY2AxisYPosition = (config) => calculateVerticalPadding(config);
 const createAxes = (axis, scale, config, canvasSVG) => {
  getAxesScale(axis, scale, config);
  prepareHAxis(scale, axis, config, prepareHorizontalAxis);
+  // eslint-disable-next-line no-use-before-define
+  const rotateAngle = config.rotateAngle ? calculateRotateAngleBasedOnWidth(config) : undefined;


This is vague. Since this is an x-axis prop, we can change this toconfig.axis.x.ticks.rotationAngle.

https://engineering.cerner.com/terra-ui/graphs/cerner-terra-graphs-docs/core-configuration/documentation/axes

resolved in 3a9d13e

sdadn · 2023-11-06T17:02:47Z

packages/carbon-graphs/src/js/helpers/axis.js

+  if (containerWidth < thresholdSmall) {
+    return -45; // rotate by -45 degrees for small screens
+  } if (containerWidth < thresholdMedium) {
+    return -30; // rotate by -30 degrees for medium screens
+  }
+  return -15; // rotate by -15 degrees for larger screens
+};


I believe the solution we agreed upon was to make the prop an integer and let consumers choose the angle as per their own requirements. What if a consumer wants to rotate it by 60º? Or what if they would like to rotate it in the other direction like positive 45º? By hardcoding the values and specifying them per screen size, we risk interpreting their data in ways they do not want it to.

resolved in 3a9d13e

@sdadn we had meetings after that meeting where we decided that we should restrict the available values to the default horizontal or -45º

cc: @mjpalazzo

sdadn · 2023-11-08T01:34:44Z

The tick values now overlap with the axis label. I think we'll need to dynamically adjust the padding of the axis label to account for the change in height.

sdadn · 2023-11-08T01:36:58Z

packages/carbon-graphs/src/js/helpers/axis.js

+    .attr('id', 'x')
+    .selectAll('text')
+    .style('text-anchor', () => {
+      if (config.axis.x.ticks.rotateAngle !== undefined) {


rotateAngle is grammatically incorrect and also unclear without reading the documentation. Can we rename this to tickLabelsRotation?

Suggested change

if (config.axis.x.ticks.rotateAngle !== undefined) {

if (config.axis.x.ticks.tickLabelsRotation !== undefined) {

sdadn · 2023-11-08T01:42:36Z

...erra-graphs-docs/src/terra-dev-site/graph/CoreConfiguration.b/Documentation/Axes.3.graph.mdx

@@ -80,7 +81,8 @@ axis: {
 | show                  | boolean        | `true`                     | no       | Toggle for showing the axis.                                                                                                                                                                                                                                          |
 | suppressTrailingZeros | boolean        | `false`                    | no       | Toggle to suppress tick values's trailing zeros when default d3 tick formatting is used. For X axis, this property works only when X axis type is set to AXIS_TYPE.DEFAULT. Specifying `~` in the tick format takes precedence over `suppressTrailingZeros` property. |
 | ticks                 | object         | `null`                     | no       | See [Ticks](./Ticks).                                                                                                                                                                                                                                                 |
-| type                  | string         | `AXIS_TYPE.DEFAULT`        | no       | See [type](#type). Normal number value or time-based. Only for x-axis.                                                                                                                                                                                                |
+| type                  | string         | `AXIS_TYPE.DEFAULT`        | no       | See [type](#type). Normal number value or time-based. Only for x-axis.                                                                                                                                                                                                |    
+| rotateAngle           | integer        | -                          | no       | allows consumer to add a rotateAngle to x-axis to avoid the overlapping (angle will be in between `+90` to `-90` Positive values move in clock wise direction & negative values moves in anticlock wise direction).Only for x-axis.                                   |


We should avoid saying "consumer" wherever possible.

Suggested change

| rotateAngle | integer | - | no | allows consumer to add a rotateAngle to x-axis to avoid the overlapping (angle will be in between `+90` to `-90` Positive values move in clock wise direction & negative values moves in anticlock wise direction).Only for x-axis. |

| rotateAngle | integer | - | no | (Only for x-axis) Sets the rotation of the tick labels from `+90º` to `-90º`. Positive values rotate the label clockwise while negative values rotate the label anti-clockwise. |

sdadn · 2023-11-08T01:53:30Z

Questions about cases:

What happens when the prop is not an integer (string, bool, float etc) ?
What happens when the values exceeds +/- 90º ?
Is unit of the angle only in degrees? What about radians?

We should add validation for the datatype in this function:

terra-graphs/packages/carbon-graphs/src/js/controls/Graph/Graph.js

Line 115 in 722464a

const beforeInit = (control) => {

eawww

The two main issues I see:

Rotation values should be limited to the default and 45º anti-clockwise (-45º). More values may be added in the future. For now, those are should be the only two values available to consumers.
The rotated values overlapping the x-axis label

supreethmr · 2023-11-09T09:04:41Z

packages/carbon-graphs/CHANGELOG.md

+* Fixed
+  * Added prop `rotateAngle` to allow rotation of x-axis tick labels to prevent overlap.


Suggested change

* Fixed

* Added prop `rotateAngle` to allow rotation of x-axis tick labels to prevent overlap.

* Fixed

* Added prop `rotateAngle` to allow rotation of x-axis tick labels to prevent overlap.

Can we add space for consistency

We should also correct the prop name

sdadn

This looks good! Just wondering if the padding is too much (cc: @eawww ) and this will also need unit tests.

sdadn · 2023-11-15T01:44:20Z

packages/carbon-graphs/CHANGELOG.md

@@ -1,6 +1,8 @@
 # ChangeLog

 ## Unreleased
+* Fixed


Suggested change

* Fixed

* Added

sdadn · 2023-11-15T01:45:43Z

packages/carbon-graphs/src/js/helpers/axis.js

@@ -1122,6 +1137,14 @@ const getXAxisHeight = (config) => {
  const dummy = d3.select('body').append('div');
  const svg = dummy.append('svg');
  const group = svg.append('g').call(axis);
+  if (config.axis.x.ticks && config.axis.x.ticks.TickLabelRotations === -45) {
+    // Add extra padding for rotated tick labels
+    const extraPadding = 5;


We should save this in the constants file for easy maintanence.

We should also get UX opinion on the appropriate padding.
cc: @eawww

sdadn · 2023-11-15T01:46:57Z

...raphs-docs/src/example-datasets/graphConfigObjects/General/generalDefaultXAxisOverlapping.js

+  showShapes: true,
+  showVGrid: true,
+  showHGrid: true,
+  locale: Carbon.helpers.LOCALE.de_DE,


Why is the locale german?

that was not intentional, I was trying to recreate the issue with tick values(date formatted) having long text in it.

sdadn · 2023-11-15T01:47:27Z

...raphs-docs/src/example-datasets/graphConfigObjects/General/generalDefaultXAxisOverlapping.js

+          new Date(2016, 0, 10).toISOString(),
+          new Date(2016, 0, 11).toISOString(),
+        ],
+        TickLabelRotations: -45,


The prop should be tickLabelsRotation.

Suggested change

TickLabelRotations: -45,

tickLabelsRotation: -45,

sdadn · 2023-11-15T01:49:39Z

packages/carbon-graphs/src/js/helpers/axis.js

  const dummy = d3.select('body').append('div');
  const svg = dummy.append('svg');
  const grouper = svg.append('g');
+
+  if (TickLabelRotations && TickLabelRotations === -45) {


Why do you need a double condition here? This should be sufficient

Suggested change

if (TickLabelRotations && TickLabelRotations === -45) {

if (TickLabelRotations === -45) {

sdadn · 2023-11-15T01:54:19Z

packages/carbon-graphs/src/js/helpers/axis.js

+      const validRotations = Object.values(utils.TickLabelRotations);
+      if (
+        utils.isDefined(config.axis.x.ticks.TickLabelRotations)
+        && validRotations.includes(config.axis.x.ticks.TickLabelRotations)


with the usage of the set, this will be

Suggested change

&& validRotations.includes(config.axis.x.ticks.TickLabelRotations)

&& validRotations.has(config.axis.x.ticks.TickLabelRotations)

sdadn · 2023-11-15T01:56:56Z

packages/carbon-graphs/CHANGELOG.md

+* Fixed
+  * Added prop `rotateAngle` to allow rotation of x-axis tick labels to prevent overlap.


We should also correct the prop name

sdadn · 2023-11-15T01:58:51Z

packages/carbon-graphs/src/js/helpers/axis.js

+    // Add extra padding for rotated tick labels
+    const extraPadding = 5;
+    group.selectAll('.tick text').attr('transform', `rotate(${config.axis.x.ticks.TickLabelRotations})`);
+    const rotatedHeight = group.node().getBoundingClientRect().height;


For clarity:

Suggested change

const rotatedHeight = group.node().getBoundingClientRect().height;

const rotatedTickLabelsHeight = group.node().getBoundingClientRect().height;

sdadn · 2023-11-15T01:59:35Z

packages/carbon-graphs/src/js/helpers/axis.js

+        && validRotations.includes(config.axis.x.ticks.TickLabelRotations)
+      ) {
+        const rotation = config.axis.x.ticks.TickLabelRotations;
+        console.warn(`tickLabelsRotation Using ${rotation} rotation for x-axis labels to avoid overlapping.`);


Why are we warning when consumers are deliberately using the prop to set the rotation?

sdadn · 2023-11-15T02:00:32Z

packages/carbon-graphs/src/js/helpers/axis.js

+        console.warn(`tickLabelsRotation Using ${rotation} rotation for x-axis labels to avoid overlapping.`);
+        return rotation === utils.TickLabelRotations.NEGATIVE_45 ? 'end' : 'middle';
+      }
+      console.warn('Warning: Invalid tickLabelsRotation angle. Using default rotation (0 or -45) for x-axis labels.');


Suggested change

console.warn('Warning: Invalid tickLabelsRotation angle. Using default rotation (0 or -45) for x-axis labels.');

console.warn('Warning: Invalid tickLabelsRotation angle. Valid values are 0 or -45');

We should also be doing this validation in the beforeInit function in Graph.js

terra-graphs/packages/carbon-graphs/src/js/controls/Graph/Graph.js

Line 115 in 722464a

const beforeInit = (control) => {

sdadn · 2023-11-15T02:10:06Z

...erra-graphs-docs/src/terra-dev-site/graph/CoreConfiguration.b/Documentation/Axes.3.graph.mdx

@@ -80,7 +81,8 @@ axis: {
 | show                  | boolean        | `true`                     | no       | Toggle for showing the axis.                                                                                                                                                                                                                                          |
 | suppressTrailingZeros | boolean        | `false`                    | no       | Toggle to suppress tick values's trailing zeros when default d3 tick formatting is used. For X axis, this property works only when X axis type is set to AXIS_TYPE.DEFAULT. Specifying `~` in the tick format takes precedence over `suppressTrailingZeros` property. |
 | ticks                 | object         | `null`                     | no       | See [Ticks](./Ticks).                                                                                                                                                                                                                                                 |
-| type                  | string         | `AXIS_TYPE.DEFAULT`        | no       | See [type](#type). Normal number value or time-based. Only for x-axis.                                                                                                                                                                                                |
+| type                  | string         | `AXIS_TYPE.DEFAULT`        | no       | See [type](#type). Normal number value or time-based. Only for x-axis.                                                                                                                                                                                                |    
+| TickLabelRotations    | integer        | -                          | no       | Sets the rotation of the tick labels to `-45º`. Only for x-axis.                                                                                                                                                                                                      |


eawww · 2023-11-15T17:02:48Z

...erra-graphs-docs/src/terra-dev-site/graph/CoreConfiguration.b/Documentation/Axes.3.graph.mdx

@@ -80,7 +81,8 @@ axis: {
 | show                  | boolean        | `true`                     | no       | Toggle for showing the axis.                                                                                                                                                                                                                                          |
 | suppressTrailingZeros | boolean        | `false`                    | no       | Toggle to suppress tick values's trailing zeros when default d3 tick formatting is used. For X axis, this property works only when X axis type is set to AXIS_TYPE.DEFAULT. Specifying `~` in the tick format takes precedence over `suppressTrailingZeros` property. |
 | ticks                 | object         | `null`                     | no       | See [Ticks](./Ticks).                                                                                                                                                                                                                                                 |
-| type                  | string         | `AXIS_TYPE.DEFAULT`        | no       | See [type](#type). Normal number value or time-based. Only for x-axis.                                                                                                                                                                                                |
+| type                  | string         | `AXIS_TYPE.DEFAULT`        | no       | See [type](#type). Normal number value or time-based. Only for x-axis.                                                                                                                                                                                                |    
+| tickLabelsRotation    | integer        | -                          | no       | Sets the rotation of the tick labels to `0º` or `-45º`. Only for x-axis.                                                                                                                                                                                              |


Shouldn't the default be 0?

Also, while the integer type implies that the corresponding integer should be used, formatting 0º and -45º as code here is misleading because those aren't the values they'll be using.

Could say something like:
Sets the rotation of the x-axis tick labels to 0º or -45º. Accepted values: 0 or -45

sdadn

Are there unit tests for this change?

sdadn · 2023-11-15T23:11:26Z

packages/carbon-graphs/src/js/controls/Graph/Graph.js

@@ -120,6 +120,13 @@ const beforeInit = (control) => {
    console.warn('allowCalibration for x-axis is a new feature that is currently a work in progress and may have stability issues. Use it at your own risk.');
    getAxesDataRange({}, constants.X_AXIS, control.config);
  }
+  if (
+    utils.isDefined(control.config.axis.x.ticks.tickLabelsRotation)
+    && utils.validTickLabelRotations.has(control.config.axis.x.ticks.tickLabelsRotation)


We want this to give a warning when a consumer uses a non-existing value

Suggested change

&& utils.validTickLabelRotations.has(control.config.axis.x.ticks.tickLabelsRotation)

&& !utils.validTickLabelRotations.has(control.config.axis.x.ticks.tickLabelsRotation)

sdadn · 2023-11-15T23:15:26Z

packages/carbon-graphs/src/js/controls/Graph/Graph.js

+    const rotation = control.config.axis.x.ticks.tickLabelsRotation;
+    console.warn(`tickLabelsRotation Using ${rotation} rotation for x-axis labels to avoid overlapping.`);


The constant is not necessary. And the message should warn when consumers use an incorrect value.

Suggested change

const rotation = control.config.axis.x.ticks.tickLabelsRotation;

console.warn(`tickLabelsRotation Using ${rotation} rotation for x-axis labels to avoid overlapping.`);

console.warn(`${control.config.axis.x.ticks.tickLabelsRotation} is an invalid value for tickLabelsRotation. Valid values are: 0, -45. Resorting to the default value of 0`);

sdadn · 2023-11-15T23:15:51Z

packages/carbon-graphs/src/js/helpers/axis.js

+        const rotation = config.axis.x.ticks.tickLabelsRotation;
+        return rotation === -45 ? 'end' : 'middle';
+      }
+      console.warn('Warning: Invalid tickLabelsRotation angle. Valid values are (0 or -45) for x-axis labels.');


we can remove this warning here

sdadn · 2023-11-15T23:18:31Z

packages/carbon-graphs/src/js/helpers/axis.js

+      if (
+        utils.isDefined(config.axis.x.ticks.tickLabelsRotation)
+        && utils.validTickLabelRotations.has(config.axis.x.ticks.tickLabelsRotation)
+      ) {
+        const rotation = config.axis.x.ticks.tickLabelsRotation;
+        return rotation === -45 ? 'end' : 'middle';


Suggested change

if (

utils.isDefined(config.axis.x.ticks.tickLabelsRotation)

&& utils.validTickLabelRotations.has(config.axis.x.ticks.tickLabelsRotation)

) {

const rotation = config.axis.x.ticks.tickLabelsRotation;

return rotation === -45 ? 'end' : 'middle';

if (config.axis.x.ticks.tickLabelsRotation === 0)

{

return 'middle' ;

packages/carbon-graphs/src/js/helpers/axis.js

sdadn · 2023-11-15T23:19:23Z

packages/carbon-graphs/src/js/controls/Graph/Graph.js

+  ) {
+    const rotation = control.config.axis.x.ticks.tickLabelsRotation;
+    console.warn(`tickLabelsRotation Using ${rotation} rotation for x-axis labels to avoid overlapping.`);
+  }


We should add a condition where if config.axis.x.ticks.tickLabelsRotation is undefined, then it should be set to 0 as the default value.

sdadn · 2023-11-15T23:19:54Z

packages/carbon-graphs/src/js/helpers/axis.js

  const dummy = d3.select('body').append('div');
  const svg = dummy.append('svg');
  const grouper = svg.append('g');
+
+  if (tickLabelsRotation === -45) {


To make it future proof for additional values:

Suggested change

if (tickLabelsRotation === -45) {

if (tickLabelsRotation !== 0) {

sdadn · 2023-11-15T23:20:10Z

packages/carbon-graphs/src/js/helpers/utils.js

+ * @readonly
+ * @enum {number}
+ */
+const validTickLabelRotations = new Set([0, -45]);


sdadn · 2023-11-15T23:20:21Z

packages/terra-graphs-docs/CHANGELOG.md

@@ -1,6 +1,9 @@
 # ChangeLog

 ## Unreleased
+* Added
+  * Added a Prop `rotateAngle` for overlapping X-axis tick values.


We can remove this entry

Suggested change

* Added a Prop `rotateAngle` for overlapping X-axis tick values.

sdadn · 2023-11-18T04:41:05Z

packages/carbon-graphs/src/js/helpers/axis.js

+      const rotation = config.axis.x.ticks.tickLabelsRotation;
+      if (rotation === 0) {
+        return 'middle';
+      } if (rotation === -45) {


Hardcoding -45 defeats the purpose of utilizing the set array. We should minimize refactoring done in the future if we need to add more angles. The conditions should be:

tickLabelsRotation = 0 -> return middle
else return end

tickLabelsRotation is undefined and tickLabelsRotation is invalid value -> tickLabelsRotation should be covered by the logic added in Graph.js.

sdadn · 2023-11-18T04:42:43Z

packages/carbon-graphs/src/js/helpers/axis.js

+      }
+      return 'middle';
+    })
+    .attr('transform', () => `rotate(${config.axis.x.ticks.tickLabelsRotation === -45 ? -45 : 0})`);


this ternary is unnecessary, we can use the value directly.

sdadn · 2023-11-18T04:43:14Z

packages/carbon-graphs/src/js/helpers/axis.js

@@ -1122,6 +1139,13 @@ const getXAxisHeight = (config) => {
  const dummy = d3.select('body').append('div');
  const svg = dummy.append('svg');
  const group = svg.append('g').call(axis);
+  if (config.axis.x.ticks && config.axis.x.ticks.tickLabelsRotation === -45) {


We shouldn't hardcode -45

Suggested change

if (config.axis.x.ticks && config.axis.x.ticks.tickLabelsRotation === -45) {

if (config.axis.x.ticks && config.axis.x.ticks.tickLabelsRotation !== 0) {

sdadn · 2023-11-18T04:43:40Z

packages/carbon-graphs/tests/unit/controls/Graph/GraphAxes-spec.js

+      graph = new Graph(getAxes(localeAxisObj));
+      expect(localeAxisObj.x.ticks.tickLabelsRotation).toBe(0);
+    });
+    fit('tickLabelsRotation values will be 0 or -45', () => {


Suggested change

fit('tickLabelsRotation values will be 0 or -45', () => {

it('tickLabelsRotation values will be 0 or -45', () => {

sdadn · 2023-11-18T04:44:22Z

packages/carbon-graphs/tests/unit/controls/Graph/GraphAxes-spec.js

+        format: '%b %Y',
+        show: true,
+        lowerStepTickValues: [
+          new Date(2017, 1).toISOString(),
+          new Date(2017, 5).toISOString(),
+          new Date(2017, 9).toISOString(),
+        ],
+        midpointTickValues: [
+          new Date(2017, 3).toISOString(),
+          new Date(2017, 7).toISOString(),
+          new Date(2017, 11).toISOString(),
+        ],
+        upperStepTickValues: [
+          new Date(2016, 11).toISOString(),
+          new Date(2018, 1).toISOString(),
+        ],


we don't need this

Suggested change

format: '%b %Y',

show: true,

lowerStepTickValues: [

new Date(2017, 1).toISOString(),

new Date(2017, 5).toISOString(),

new Date(2017, 9).toISOString(),

],

midpointTickValues: [

new Date(2017, 3).toISOString(),

new Date(2017, 7).toISOString(),

new Date(2017, 11).toISOString(),

],

upperStepTickValues: [

new Date(2016, 11).toISOString(),

new Date(2018, 1).toISOString(),

],

format: '%b %Y',

show: true,

sdadn · 2023-11-18T04:44:43Z

packages/carbon-graphs/tests/unit/controls/Graph/GraphAxes-spec.js

+      // Call the code that logs the warning
+      graph = new Graph(getAxes(localeAxisObj));
+
+      expect(console.warn).toHaveBeenCalledWith('23 is an invalid value for tickLabelsRotation. Valid values are: 0, -45. Resorting to the default value of 0');


sdadn · 2023-11-18T04:45:28Z

packages/carbon-graphs/tests/unit/controls/Graph/GraphAxes-spec.js

+      graph = new Graph(getAxes(localeAxisObj));
+      expect(localeAxisObj.x.ticks.tickLabelsRotation).toBe(-45);
+    });
+    fit('tickLabelsRotation default zero', () => {


Suggested change

fit('tickLabelsRotation default zero', () => {

it('uses the default value of zero if tickLabelsRotation it is invalid', () => {

sdadn · 2023-11-18T04:46:18Z

packages/carbon-graphs/tests/unit/controls/Graph/GraphAxes-spec.js

+      // Call the code that logs the warning
+      graph = new Graph(getAxes(localeAxisObj));
+
+      expect(console.warn).toHaveBeenCalledWith('23 is an invalid value for tickLabelsRotation. Valid values are: 0, -45. Resorting to the default value of 0');


We should also test it tickLabelsRotation is 0

sdadn · 2023-11-18T04:48:21Z

packages/carbon-graphs/tests/unit/controls/Graph/GraphAxes-spec.js

+        tickLabelsRotation: 0,
+      };
+      graph = new Graph(getAxes(localeAxisObj));
+      expect(localeAxisObj.x.ticks.tickLabelsRotation).toBe(0);


localeAxisObj is input. We should test for the graphs object in all test cases.

Suggested change

expect(localeAxisObj.x.ticks.tickLabelsRotation).toBe(0);

expect(graph.config.axis.x.ticks.tickLabelsRotation).toBe(0);

sdadn · 2023-11-20T19:20:58Z

packages/carbon-graphs/src/js/helpers/axis.js

+      const rotation = config.axis.x.ticks.tickLabelsRotation;
+      if (rotation === 0) {
+        return 'middle';
+      } if (rotation < 0) {


Angles can be positive too

Suggested change

} if (rotation < 0) {

} if (rotation !== 0) {

sdadn · 2023-11-20T19:21:47Z

packages/carbon-graphs/tests/unit/controls/Graph/GraphAxes-spec.js

+    it('tickLabelsRotation values will be 0 or -45', () => {
+      const localeAxisObj = utils.deepClone(axisTimeSeries);
+      localeAxisObj.x = {
+        type: 'timeseries',
+        label: 'Some X Label',
+        lowerLimit: new Date(2017, 0).toISOString(),
+        upperLimit: new Date(2017, 6).toISOString(),
+      };
+      localeAxisObj.x.ticks = {
+        tickLabelsRotation: 0,
+      };
+      graph = new Graph(getAxes(localeAxisObj));
+      expect(graph.config.axis.x.ticks.tickLabelsRotation).toBe(0);
+    });
+    it('tickLabelsRotation values will be 0 or -45', () => {
+      const localeAxisObj = utils.deepClone(axisTimeSeries);
+      localeAxisObj.x = {
+        type: 'timeseries',
+        label: 'Some X Label',
+        lowerLimit: new Date(2017, 0).toISOString(),
+        upperLimit: new Date(2017, 6).toISOString(),
+      };
+      localeAxisObj.x.ticks = {
+        tickLabelsRotation: -45,
+      };
+      graph = new Graph(getAxes(localeAxisObj));
+      expect(graph.config.axis.x.ticks.tickLabelsRotation).toBe(-45);
+    });


Why do these tests have the same title?

sdadn · 2023-11-20T19:21:56Z

packages/carbon-graphs/tests/unit/controls/Graph/GraphAxes-spec.js

+      graph = new Graph(getAxes(localeAxisObj));
+      expect(graph.config.axis.x.ticks.tickLabelsRotation).toBe(-45);
+    });
+    it('uses the default value of zero if tickLabelsRotation it is invalid', () => {


Suggested change

it('uses the default value of zero if tickLabelsRotation it is invalid', () => {

it('uses the default value of zero of tickLabelsRotation it is invalid', () => {

sdadn · 2023-11-20T19:23:12Z

...-graphs-docs/src/terra-dev-site/test/graphs/General/Axes/XAxisTickValuesOverlapping.test.jsx

Did you generate wdio screenshots for this test?

sdadn · 2023-11-20T19:23:39Z

packages/carbon-graphs/src/js/helpers/constants.js

@@ -316,4 +316,5 @@ export default {
  DEFAULT_LEGEND_LINE_WIDTH: 28,
  DEFAULT_LEGEND_LINE_WIDTH_WITH_SYMBOL: 18,
  LEGEND_LINE_POSITION: 1.5,
+  DEFAULT_OVERLAPPING_PADDING: 5,


This needs to be reviewed by UX:
cc: @eawww

Assuming this is relative to the bottom of the tick labels, the typical value seems to be half of this. I'd like to see what 2.5 looks like.

sdadn · 2023-11-20T19:25:25Z

packages/carbon-graphs/tests/unit/controls/Graph/GraphAxes-spec.js

+
+      expect(graph.config.axis.x.ticks.tickLabelsRotation).toBe(0);
+    });
+    it('uses the default value of zero if tickLabelsRotation it is undefind', () => {


Suggested change

it('uses the default value of zero if tickLabelsRotation it is undefind', () => {

it('uses the default value of zero if tickLabelsRotation is undefind', () => {

sdadn

Since the above comments are minor and don't break functionality, I'm approving this for now assuming that they get resolved.

eawww

This looks like it still needs a consumer facing example. The tests don't appear on the main aggregated doc site.

eawww

The "No Data" appears to be in Spanish. It appears in other languages on other examples. Is this a fun quirky feature or an i18n issue?

eawww · 2023-11-21T15:49:10Z

...rra-dev-site/graph/CoreConfiguration.b/Examples/Axes.1/XAxisTickLabelOrientation.8.graph.mdx

+
+# X-Axis Tick Label orientation
+
+This is a simple timeseries line graph with x-axis ticks label orientation `tickLabelsRotation = -45`.


This is a simple timeseries line graph with x-axis ticks label orientation `tickLabelsRotation = -45`. Tick label rotation is used to prevent labels from overlapping in a narrow viewport or due to long tick labels.

eawww · 2023-11-21T16:38:02Z

packages/carbon-graphs/src/js/helpers/constants.js

@@ -316,4 +316,5 @@ export default {
  DEFAULT_LEGEND_LINE_WIDTH: 28,
  DEFAULT_LEGEND_LINE_WIDTH_WITH_SYMBOL: 18,
  LEGEND_LINE_POSITION: 1.5,
+  DEFAULT_OVERLAPPING_PADDING: 5,


Assuming this is relative to the bottom of the tick labels, the typical value seems to be half of this. I'd like to see what 2.5 looks like.

supreethmr · 2023-11-21T19:32:37Z

packages/carbon-graphs/src/js/controls/Graph/Graph.js

+  if (!utils.isDefined(control.config.axis.x.ticks.tickLabelsRotation)) {
+    control.config.axis.x.ticks.tickLabelsRotation = 0;
+  } else if (!utils.validTickLabelRotations.has(control.config.axis.x.ticks.tickLabelsRotation)) {
+    console.warn(`${control.config.axis.x.ticks.tickLabelsRotation} is an invalid value for tickLabelsRotation. Valid values are: 0, -45. Resorting to the default value of 0`);
+    control.config.axis.x.ticks.tickLabelsRotation = 0;
+  }


Suggested change

if (!utils.isDefined(control.config.axis.x.ticks.tickLabelsRotation)) {

control.config.axis.x.ticks.tickLabelsRotation = 0;

} else if (!utils.validTickLabelRotations.has(control.config.axis.x.ticks.tickLabelsRotation)) {

console.warn(`${control.config.axis.x.ticks.tickLabelsRotation} is an invalid value for tickLabelsRotation. Valid values are: 0, -45. Resorting to the default value of 0`);

control.config.axis.x.ticks.tickLabelsRotation = 0;

}

const isInvalidTickLabelRotations = !utils.isDefined(control.config.axis.x.ticks.tickLabelsRotation) || !utils.validTickLabelRotations.has(control.config.axis.x.ticks.tickLabelsRotation);

if (isInvalidTickLabelRotations) {

control.config.axis.x.ticks.tickLabelsRotation = 0;

console.warn(`${control.config.axis.x.ticks.tickLabelsRotation} is an invalid value for tickLabelsRotation. Valid values are: 0, -45. Resorting to the default value of 0`);

}

Can we simplify like above ?

supreethmr · 2023-11-21T19:35:08Z

packages/carbon-graphs/src/js/helpers/axis.js

+  if (tickLabelsRotation !== 0) {
+    // Adding extra padding for rotated labels
+    grouper.attr('transform', `rotate(${tickLabelsRotation})`);
+  }


Suggested change

if (tickLabelsRotation !== 0) {

// Adding extra padding for rotated labels

grouper.attr('transform', `rotate(${tickLabelsRotation})`);

}

if (tickLabelsRotation) {

// Adding extra padding for rotated labels

grouper.attr('transform', `rotate(${tickLabelsRotation})`);

}

supreethmr · 2023-11-21T19:37:03Z

packages/terra-graphs-docs/CHANGELOG.md

 ## Unreleased
+* Added
+  * Added examples for the Xaxis Overlapping tick values.


Suggested change

## Unreleased

* Added

* Added examples for the Xaxis Overlapping tick values.

## Unreleased

* Added

* Added examples for the X Axis Overlapping tick values.

Need a line break for consistency !

eawww

Met with @udaymattam and discussed the last changes needed:

Space between the bottom of the x-axis tick labels and the x-axis label should be half of what it is now to be consistent with other examples.
There should be a test to ensure that, when the Legend is present at the bottom, it is still positioned the same relative to the x-axis label.

Since I'll be out until next week for the US holiday, putting conditional approval on here so long as the two above changes are made.

eawww · 2023-11-22T16:21:45Z

Worked with Uday to fine tune the padding between the tick labels and axis labels. Here is a screenshot of the result

We also ensured the legend still had appropriate padding. After these changes, this looks good to me!

Added prop rotateAngle

ade981b

udaymattam requested a review from sdadn November 6, 2023 10:54

udaymattam self-assigned this Nov 6, 2023

github-actions bot temporarily deployed to preview-pr-322 November 6, 2023 10:54 Destroyed

udaymattam requested review from supreethmr, saket2403 and ShettyAkarsh November 6, 2023 10:54

sdadn reviewed Nov 6, 2023

View reviewed changes

sdadn added the 📦 terra-graphs label Nov 6, 2023

prop type changed as integer

3a9d13e

github-actions bot temporarily deployed to preview-pr-322 November 7, 2023 10:45 Destroyed

sdadn added the UX Review Required label Nov 7, 2023

sdadn reviewed Nov 8, 2023

View reviewed changes

eawww reviewed Nov 8, 2023

View reviewed changes

supreethmr reviewed Nov 9, 2023

View reviewed changes

enum[0,-45] rotations added

756d613

github-actions bot temporarily deployed to preview-pr-322 November 14, 2023 08:43 Destroyed

sdadn reviewed Nov 15, 2023

View reviewed changes

updated comments

79d79eb

github-actions bot temporarily deployed to preview-pr-322 November 15, 2023 10:07 Destroyed

eawww reviewed Nov 15, 2023

View reviewed changes

sdadn reviewed Nov 15, 2023

View reviewed changes

default zero rotation

f0183ce

github-actions bot temporarily deployed to preview-pr-322 November 17, 2023 12:53 Destroyed

sdadn reviewed Nov 18, 2023

View reviewed changes

updated test & resolved commets

beb094a

github-actions bot temporarily deployed to preview-pr-322 November 20, 2023 14:50 Destroyed

clean code

bc666bc

github-actions bot temporarily deployed to preview-pr-322 November 20, 2023 15:35 Destroyed

sdadn reviewed Nov 20, 2023

View reviewed changes

sdadn approved these changes Nov 20, 2023

View reviewed changes

eawww reviewed Nov 20, 2023

View reviewed changes

consumer facing example

cf44461

github-actions bot temporarily deployed to preview-pr-322 November 21, 2023 15:24 Destroyed

eawww reviewed Nov 21, 2023

View reviewed changes

supreethmr reviewed Nov 21, 2023

View reviewed changes

constants

95fdf8a

github-actions bot temporarily deployed to preview-pr-322 November 22, 2023 15:33 Destroyed

eawww approved these changes Nov 22, 2023

View reviewed changes

legend & label padding

e57dbf0

github-actions bot temporarily deployed to preview-pr-322 November 23, 2023 05:32 Destroyed

supreethmr approved these changes Nov 24, 2023

View reviewed changes

udaymattam merged commit f13b8ba into main Nov 24, 2023
9 checks passed

udaymattam deleted the x-axis-overlaping branch November 24, 2023 11:15

		* Fixed
		* Fixed tick values overlapping on X-axis by rotating 15,30,45.

	if (config.axis.x.ticks.rotateAngle !== undefined) {
	if (config.axis.x.ticks.tickLabelsRotation !== undefined) {

	\| rotateAngle \| integer \| - \| no \| allows consumer to add a rotateAngle to x-axis to avoid the overlapping (angle will be in between `+90` to `-90` Positive values move in clock wise direction & negative values moves in anticlock wise direction).Only for x-axis. \|
	\| rotateAngle \| integer \| - \| no \| (Only for x-axis) Sets the rotation of the tick labels from `+90º` to `-90º`. Positive values rotate the label clockwise while negative values rotate the label anti-clockwise. \|

		* Fixed
		* Added prop `rotateAngle` to allow rotation of x-axis tick labels to prevent overlap.

	if (TickLabelRotations && TickLabelRotations === -45) {
	if (TickLabelRotations === -45) {

	&& validRotations.includes(config.axis.x.ticks.TickLabelRotations)
	&& validRotations.has(config.axis.x.ticks.TickLabelRotations)

	const rotatedHeight = group.node().getBoundingClientRect().height;
	const rotatedTickLabelsHeight = group.node().getBoundingClientRect().height;

	console.warn('Warning: Invalid tickLabelsRotation angle. Using default rotation (0 or -45) for x-axis labels.');
	console.warn('Warning: Invalid tickLabelsRotation angle. Valid values are 0 or -45');

	\| TickLabelRotations \| integer \| - \| no \| Sets the rotation of the tick labels to `-45º`. Only for x-axis. \|
	\| TickLabelRotations \| integer \| - \| no \| Sets the rotation of the tick labels to `0º` or `-45º`. Only for x-axis. \|

	&& utils.validTickLabelRotations.has(control.config.axis.x.ticks.tickLabelsRotation)
	&& !utils.validTickLabelRotations.has(control.config.axis.x.ticks.tickLabelsRotation)

		const rotation = control.config.axis.x.ticks.tickLabelsRotation;
		console.warn(`tickLabelsRotation Using ${rotation} rotation for x-axis labels to avoid overlapping.`);

	const rotation = control.config.axis.x.ticks.tickLabelsRotation;
	console.warn(`tickLabelsRotation Using ${rotation} rotation for x-axis labels to avoid overlapping.`);
	console.warn(`${control.config.axis.x.ticks.tickLabelsRotation} is an invalid value for tickLabelsRotation. Valid values are: 0, -45. Resorting to the default value of 0`);

	if (tickLabelsRotation === -45) {
	if (tickLabelsRotation !== 0) {

	if (config.axis.x.ticks && config.axis.x.ticks.tickLabelsRotation === -45) {
	if (config.axis.x.ticks && config.axis.x.ticks.tickLabelsRotation !== 0) {

	fit('tickLabelsRotation values will be 0 or -45', () => {
	it('tickLabelsRotation values will be 0 or -45', () => {

	fit('tickLabelsRotation default zero', () => {
	it('uses the default value of zero if tickLabelsRotation it is invalid', () => {

	expect(localeAxisObj.x.ticks.tickLabelsRotation).toBe(0);
	expect(graph.config.axis.x.ticks.tickLabelsRotation).toBe(0);

	it('uses the default value of zero if tickLabelsRotation it is invalid', () => {
	it('uses the default value of zero of tickLabelsRotation it is invalid', () => {


		# X-Axis Tick Label orientation

		This is a simple timeseries line graph with x-axis ticks label orientation `tickLabelsRotation = -45`.

added prop rotateAngle to avoid the overlapping x-axis tick values #322

added prop rotateAngle to avoid the overlapping x-axis tick values #322

Conversation

udaymattam commented Nov 6, 2023 • edited Loading

Summary

Testing

Reviews

Additional Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn commented Nov 8, 2023

eawww left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn left a comment

Choose a reason for hiding this comment

eawww left a comment

Choose a reason for hiding this comment

eawww left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eawww left a comment

Choose a reason for hiding this comment

eawww commented Nov 22, 2023

udaymattam commented Nov 6, 2023 •

edited

Loading