Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

@pranav300
Copy link
Contributor

commented Sep 18, 2019

What is the purpose of this pull request? (put an "X" next to item)

  • Bug fix
  • Documentation update
  • New functionality
  • Changes an existing functionality
  • Other, please explain:

Add any screenshots

Before

before

After

after

@pranav300 pranav300 self-assigned this Sep 18, 2019
pranav300 added 2 commits Sep 19, 2019
… use-customPadding-similar-to-constantPadding
: 20) + config.padding.right
);
};
const getY2AxisWidth = (config) =>

This comment has been minimized.

Copy link
@abhijit945

abhijit945 Sep 19, 2019

Member

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

config.axisSizes.y = config.padding.hasCustomPadding
? getYAxisWidth(constants.Y_AXIS, config)
: getYAxisWidth(constants.Y_AXIS, config) + config.padding.left;
config.axisSizes.y =

This comment has been minimized.

Copy link
@abhijit945

abhijit945 Sep 19, 2019

Member

@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)?

This comment has been minimized.

Copy link
@Dinesh94Singh

Dinesh94Singh Sep 19, 2019

Collaborator

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.

@abhijit945 abhijit945 changed the title customPadding must be handled similar to constantPadding Fix: Updated axis.js to use customPadding as constantPadding (fixes #22) Sep 19, 2019
@abhijit945 abhijit945 added bug and removed enhancement labels Sep 19, 2019
@abhijit945 abhijit945 added this to In progress in Graph API Advancements via automation Sep 19, 2019
@abhijit945

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

When using the default padding values using padding property, it looks good on Timeline

image

vs

image

Aside from label getting cut-off (#81)

@Dinesh94Singh

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2019

Curious to know why custom padding is getting removed and introducing constant padding. I thought we landed upon removing the constant padding altogether and do the calculations based on custom padding field itself.

https://github.cerner.com/Geneva/carbon-graphs/issues/155

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

This comment has been minimized.

Copy link
@Dinesh94Singh

Dinesh94Singh Sep 19, 2019

Collaborator

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

This comment has been minimized.

Copy link
@pranav300

pranav300 Sep 20, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@Dinesh94Singh

Dinesh94Singh Sep 23, 2019

Collaborator

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

pranav300 added 2 commits Sep 20, 2019
… use-customPadding-similar-to-constantPadding
@pranav300

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

I am closing this PR. And I have raised a new PR #97 to resolve the issue.

@pranav300 pranav300 closed this Oct 2, 2019
Graph API Advancements automation moved this from In progress to Done Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.