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.

Sign up for GitHub

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

Jump to bottom

Added bar overlap property for 2d bar charts #667

Merged
merged 8 commits into from
Aug 11, 2021
Merged

Added bar overlap property for 2d bar charts #667

merged 8 commits into from
Aug 11, 2021

Conversation

mindline-analytics
Copy link
Contributor

No description provided.

@auto-assign auto-assign bot requested a review from Progi1984 August 9, 2021 15:22
Progi1984
Progi1984 reviewed Aug 9, 2021
View reviewed changes
Copy link
Member

@Progi1984 Progi1984 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It's a great job. Could you replace overlapWidthPercent by overlap ?

Don't forget to add your change to 1.0.0.md changelog.

Could you give a screenshot of the old/new display with this method ?

@Progi1984 Progi1984 changed the title Feat: adds bar overlap property for 2d bar charts Added bar overlap property for 2d bar charts Aug 9, 2021
@mindline-analytics
Copy link
Contributor Author

mindline-analytics commented Aug 9, 2021 •
edited
Loading

Hi, thanks for the quick review!

By replacing overlapWidthPercent through overlap you mean the method names?

By screenshot you mean of a bar chart with/ without the overlap? And where should these be added?
Like this?
image

I'll add the change to the changelog.

@Progi1984
Copy link
Member

@mindline-analytics Thank you for your contribution.

Affirmative for replacing overlapWidthPercent through overlap you mean the method and attribute names.

Do you think you can add the screenshot to the documentation ?

@mindline-analytics
Copy link
Contributor Author

Marriages have probably already failed because of methods and property naming.

Is there a plan to change the method/property name for the gap width? Since this is called gapWidthPercent, I would otherwise - for the sake of consistency - suggest to leave the naming as is. But it's your decision, if desired, I'll be happy to change it of course. :)

@Progi1984
Copy link
Member

@mindline-analytics OK for keeping naming. Last feedback on paths for images and I merge it.

@mindline-analytics
Copy link
Contributor Author

mindline-analytics commented Aug 10, 2021 •
edited
Loading

@Progi1984: Why did you revert the image paths? Is this related to the mkdocs process? I'm not familiar with it, but currently the image sources are broken in github and github.io.

@Progi1984 Progi1984 merged commit 6a57c84 into PHPOffice:develop Aug 11, 2021
@Progi1984
Copy link
Member

@mindline-analytics You're right. Thank you for your contribution.

@Progi1984 Progi1984 added this to the 1.0.0 milestone Aug 11, 2021
@mindline-analytics mindline-analytics deleted the feat-bar-overlap branch August 27, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Progi1984 Progi1984 Progi1984 approved these changes

Assignees

@Progi1984 Progi1984

Labels
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@mindline-analytics @Progi1984