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

Layout Configuration #156 #157

Closed
wants to merge 0 commits into from
Closed

Conversation

ipax77
Copy link
Contributor

@ipax77 ipax77 commented Oct 17, 2020

Please let me know if this is the correct way to implement new features. I'd like to do more pull requests, ChartJS is cool ;)

@Joelius300
Copy link
Contributor

Thanks for the Pull Request.

I see you've pushed a new commit to fix #170. When working with PRs, you should do you work in pieces, like you probably do in your own development environment too. One PR per feature/fix/whatever (usually that lines up pretty well with one PR per issue).
Since this fix has nothing to do with the layout configuration, you should create a new branch and submit a new PR with just the fix for #170. That way it's easier to review and merge.

The fix is simple and I think we can merge it in fast but could you make sure to indent the if correctly and leave one empty line before the nested if (so between config.options.scale.ticks.callback = ..... and if (!config.opti...) please?

The layout configuration I'd have to take a look at when I have more time but there are some issues like the lowercase property names and the explicit default value.

To get a clean history on this PR, you should reset your branch and create a single commit for adding the layout configuration. Then rebase to the current master and force push (that's fine since it's on a separate branch of your fork). After that it should be ready for review. If you're interested, you can do that all in one single interactive rebase :)

@ipax77
Copy link
Contributor Author

ipax77 commented Dec 19, 2020

Ok, ty for clarification! I think I got it

@Joelius300
Copy link
Contributor

Thanks for #171 👍🏼

If you want you can submit a new PR for the layout configuration and I'll take a look at it when I get time. Also you closed this PR so you don't really need the rebase stuff. I said that in case you wanted to continue using this PR for the layout configuration but now you've reset it to master so the changes are gone. In case you need to recover them.. :)

@ipax77
Copy link
Contributor Author

ipax77 commented Dec 19, 2020

Thx again for ur patience, I am new to this pull request thing ..
Now, my workflow is keep the forks master in sync with this master and submit pull requests from fork branches. And I have a branch in the fork (pax) with all my branches merged.
What I am missing then is the check if there are merge conflicts to the master, but for theses simple tasks it should be fine I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants