Skip to content
This repository has been archived by the owner on Jan 29, 2021. It is now read-only.

Fix: Updated axis.js to use customPadding as constantPadding (fixes #22) #83

Closed
wants to merge 7 commits into from
Closed

Fix: Updated axis.js to use customPadding as constantPadding (fixes #22) #83

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
245799f
Fix: Updated axis.js to use customPadding similarly as constantPadding
pranav300 Sep 18, 2019
2b156df
Fix: Updated axis.js to use customPadding as constantPadding (fixes #22)
pranav300 Sep 18, 2019
afa2810
Merge branch 'use-customPadding-similar-to-constantPadding' of https:…
pranav300 Sep 18, 2019
21a61ab
Merge branch 'master' of https://github.com/cerner/carbon-graphs into…
pranav300 Sep 19, 2019
2642165
Update: updated graph-spec
pranav300 Sep 19, 2019
361c0a1
Merge branch 'master' of https://github.com/cerner/carbon-graphs into…
pranav300 Sep 20, 2019
fd712a6
Update: Updated top padding and gantt (fixes #22) (fixes #81)
pranav300 Sep 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/main/js/controls/Timeline/helpers/creationHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,10 @@ const getXAxisYPosition = (config) =>
* @returns {number} X Axis width
*/
const getXAxisWidth = (config) =>
config.canvasWidth - config.padding.left - config.padding.right - getXAxisYPosition(config);
config.canvasWidth -
config.padding.left -
config.padding.right -
getXAxisYPosition(config);
/**
* X Axis label's starting position below the graph
*
Expand Down
24 changes: 5 additions & 19 deletions src/main/js/helpers/axis.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,9 +577,6 @@ const getYAxisHeight = (config) => config.height;
* @returns {number} Height of the X Axis ticks, labels and numbers/datetimes
*/
const getXAxisHeight = (config) => {
if (config.padding.hasCustomPadding) {
return config.padding.bottom;
Copy link

Choose a reason for hiding this comment

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

If I remember right - removing this might break Gantt with customPadding set to all 0's. Can you post a screenshot of what happens for Gantt having 0 custom padding

Copy link
Contributor Author

@pranav300 pranav300 Sep 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Screenshot 2019-09-20 at 11 22 45 AM

It doesn't break the code.
But I am not able to understand, that why are we using tracklabel to define padding left for both custom and constant GanttConfig.js

Copy link

Choose a reason for hiding this comment

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

The reason might be because, track labels are displayed in the left and the length of the trackLabel defines where to start the Gantt (since we wouldn't want our labels to get cut off - giving arbitrary values).

}
const scale = getScale(config.axis.x.type)
.domain(config.axis.x.domain)
.range([0, config.canvasWidth]);
Expand Down Expand Up @@ -654,9 +651,6 @@ const getAxisLabelHeight = (label) => {
* @returns {number} label width
*/
const getYAxisWidth = (id, config) => {
if (config.padding.hasCustomPadding) {
return config.padding.left;
}
const scale = d3.scale
.linear()
.domain([
Expand All @@ -683,16 +677,9 @@ const getYAxisWidth = (id, config) => {
* @param {object} config - config object derived from input JSON.
* @returns {number} label width
*/
const getY2AxisWidth = (config) => {
if (config.padding.hasCustomPadding) {
return config.padding.right;
}
return (
(hasY2Axis(config.axis)
? getYAxisWidth(constants.Y2_AXIS, config)
: 20) + config.padding.right
);
};
const getY2AxisWidth = (config) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes look great in Graph construct but for Gantt construct its still a bit broken. I bet that's because we are using hasCustomPadding property there as well.

With adding the property as

top: 10,
bottom: 5,
left: 30,
right: 50

Gantt
Gantt with padding property used:
image

Gantt without padding property used:
image

(hasY2Axis(config.axis) ? getYAxisWidth(constants.Y2_AXIS, config) : 20) +
config.padding.right;
/**
* Checks if X Axis orientation is set to top
*
Expand All @@ -714,9 +701,8 @@ const isXAxisOrientationTop = (xAxisOrientation) =>
*/
const calculateAxesSize = (config) => {
config.axisSizes = {};
config.axisSizes.y = config.padding.hasCustomPadding
? getYAxisWidth(constants.Y_AXIS, config)
: getYAxisWidth(constants.Y_AXIS, config) + config.padding.left;
config.axisSizes.y =
Copy link
Contributor

Choose a reason for hiding this comment

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

@Dinesh94Singh Why did we need the hasCustomPadding flag? I am looking for some background.
Can we remove it altogether from all the constructs (Graph, Gantt and Timeline)?

Copy link

Choose a reason for hiding this comment

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

it is to determine if the customPadding is given as input.

inside getPadding function, we return the padding object from constants or default few of the fields to constants when not all fields are given as part of custom padding.

Since config.padding is assigned to the return value of getPadding, there is no way to distinguish if the padding is a custom padding or constant padding.

getYAxisWidth(constants.Y_AXIS, config) + config.padding.left;
config.axisSizes.y2 = getY2AxisWidth(config);
config.axisSizes.x = getXAxisHeight(config);
};
Expand Down
16 changes: 8 additions & 8 deletions src/test/unit/controls/Graph/Graph-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,7 +1202,7 @@ describe("Graph", () => {
`.${styles.contentContainer}`
);
expect(toNumber(contentContainer.attr("x"), 10)).toBeCloserTo(
0
21
);
expect(toNumber(contentContainer.attr("y"), 10)).toBeCloserTo(
graphConfig.padding.bottom
Expand All @@ -1213,7 +1213,7 @@ describe("Graph", () => {
graph.config.axisLabelWidths.y,
10
)
).toBeCloserTo(0);
).toBeCloserTo(21);
expect(getYAxisHeight(graph.config)).toBeCloserTo(267);
});
it("Resizes correctly after rendering", (done) => {
Expand Down Expand Up @@ -1260,7 +1260,7 @@ describe("Graph", () => {
`.${styles.contentContainer}`
);
expect(toNumber(contentContainer.attr("x"), 10)).toBeCloserTo(
0
21
);
expect(toNumber(contentContainer.attr("y"), 10)).toBeCloserTo(
toNumber(
Expand All @@ -1286,7 +1286,7 @@ describe("Graph", () => {
`.${styles.contentContainer}`
);
expect(toNumber(contentContainer.attr("x"), 10)).toBeCloserTo(
0
21
);
expect(toNumber(contentContainer.attr("y"), 10)).toBeCloserTo(
graphConfig.padding.top
Expand Down Expand Up @@ -2604,7 +2604,7 @@ describe("Graph", () => {
);
expect(
toNumber(contentContainer.getAttribute("x"), 10)
).toBeCloserTo(0);
).toBeCloserTo(21);
expect(
toNumber(contentContainer.getAttribute("y"), 10)
).toBeCloserTo(0);
Expand Down Expand Up @@ -2677,7 +2677,7 @@ describe("Graph", () => {
);
expect(
toNumber(contentContainer.attr("width"), 10)
).toBeCloserTo(762);
).toBeCloserTo(712);
expect(
toNumber(contentContainer.attr("height"))
).toBeCloserTo(250);
Expand Down Expand Up @@ -2785,7 +2785,7 @@ describe("Graph", () => {
`.${styles.contentContainer}`
);
expect(toNumber(contentContainer.attr("x"), 10)).toBeCloserTo(
0
21
);
expect(toNumber(contentContainer.attr("y"), 10)).toBeCloserTo(
0
Expand All @@ -2796,7 +2796,7 @@ describe("Graph", () => {
graph.config.axisLabelWidths.y,
10
)
).toBeCloserTo(0);
).toBeCloserTo(21);
expect(getYAxisHeight(graph.config)).toBeCloserTo(267);
});
it("Renders correctly on another resize", (done) => {
Expand Down
3 changes: 2 additions & 1 deletion src/test/unit/controls/Timeline/Timeline-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,8 @@ describe("Timeline", () => {
});
it("Creates defs element with height and width", () => {
const currentWidth =
constants.PADDING.left + constants.PADDING.right +
constants.PADDING.left +
constants.PADDING.right +
(constants.PADDING.top + constants.PADDING.bottom) * 2;
const defsElement = fetchElementByClass(styles.canvas).firstChild;
expect(defsElement.nodeName).toBe("defs");
Expand Down