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

Set contenteditable=false on editor destroy() when enableContentEditable is true #356

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@mattleff
Contributor

mattleff commented Oct 12, 2015

This complements the behavior on editor creation.

@ipeychev

This comment has been minimized.

Contributor

ipeychev commented Oct 13, 2015

Hey @mattleff,

Thank you for the contribution! This PR looks good, some note about the commit rules we have:

  1. Each commit should be associated with an issue. We do that by adding Fixes #xxx in the commit message. In this case we can track any comments related to the issue, etc.
  2. The commits should have tests, except if technically impossible. We do have tests for destroying the editor, you may add another one which covers the change in your PR.

Thanks again for the contribution!

Iliyan

@ipeychev

This comment has been minimized.

Contributor

ipeychev commented Oct 14, 2015

Just started reviewing :)

:octocat: Sent from GH.

@ipeychev ipeychev added this to the 0.6 milestone Oct 14, 2015

@ipeychev

This comment has been minimized.

Contributor

ipeychev commented Oct 14, 2015

Thank you, pull request merged! See changes here.

:octocat: Sent from GH.

@ipeychev ipeychev closed this in 8858d57 Oct 14, 2015

ipeychev added a commit that referenced this pull request Oct 14, 2015

ipeychev added a commit that referenced this pull request Oct 14, 2015

ipeychev added a commit that referenced this pull request Oct 14, 2015

@ipeychev

This comment has been minimized.

Contributor

ipeychev commented Oct 14, 2015

Okay, I added the needed tests, but please the next time send us PR with tests. If you don't know how to do them, feel free to ask for help.

Thanks,

@mattleff

This comment has been minimized.

Contributor

mattleff commented Oct 14, 2015

@ipeychev Sorry I hadn't had a chance to work on unit tests! Do you have a page that explains how to run the unit tests? I have a couple more pulls I'd like to send and I can do the unit tests if I know how.

@ipeychev

This comment has been minimized.

Contributor

ipeychev commented Oct 14, 2015

Hey @mattleff,

The tests we run simple with with gulp test. I have on my machine (Mac) installed VMs for all the supported IE versions. I used ievms to install them.

Hope that helps!

@mattleff mattleff deleted the SimpleUpdates:destroy-contenteditable branch Oct 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment