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

Added updateSketchCondition as a Draw Interaction option #7535

Closed
wants to merge 3 commits into from
Closed

Added updateSketchCondition as a Draw Interaction option #7535

wants to merge 3 commits into from

Conversation

bjornharrtell
Copy link
Contributor

@bjornharrtell bjornharrtell commented Dec 4, 2017

  • This pull request addresses an issue that has been marked with the 'Pull request accepted' label & I have added the link to that issue.
  • It contains one or more small, incremental, logically separate commits, with no merge commits.
  • I have used clear commit messages.
  • Existing tests pass for me locally & I have added or updated tests for new or changed functionality.
  • The work herein is covered by a valid Contributor License Agreement (CLA).

@bjornharrtell
Copy link
Contributor Author

Replaces #4668. Trivial addition but should be unit tested, I realize now due to the nice reminders in the new PR template.

* @type {ol.EventsConditionType|undefined}
* @api
*/
olx.interaction.DrawOptions.prototype.updateSketchCondition;
Copy link
Member

Choose a reason for hiding this comment

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

The new option must also be added in the olx.interaction.DrawOptions @typedef above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing, fixed by b53d61d.

@bjornharrtell
Copy link
Contributor Author

bjornharrtell commented Dec 4, 2017

This PR is a suggested fix for the issue #3907 which was created before the "Pull request accepted" label was introduced.

@bjornharrtell
Copy link
Contributor Author

I'm not sure CLA was established for @Daanoz. However, the changes are quite trivial.

@ahocevar
Copy link
Member

ahocevar commented Dec 4, 2017

Good point. @Daanoz, can you please sign a CLA? Thanks!

@Daanoz
Copy link

Daanoz commented Dec 4, 2017

Hi, thx for the interest in closing this PR. Since I've submitted this PR (and it's fix) as part of an enterprise application, I also rather get the CLA signed by the enterprise rather than me personally signing it. But, that has proven to be quite the challenge, the related ticket has been on our own scrumboard for over an year now, so I can imagine the pain with having this ticket open for so long. Therefore I propose to just close this ticket and the branch, and see if someone else is willing to pick it up in the future, otherwise we're stuck in this deadlock?

@bjornharrtell
Copy link
Contributor Author

I would like to pick it up and start from scratch, but it will likely result in mostly identical implementation - not sure if that's ok or not. Also, I've tried to make unit tests but got stuck on the event simulation, so I'll need some more time on it. Not expecting to get get it done until after 4.6 and so will close this.

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.

None yet

4 participants