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

Protection against current code-block removing #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

newsiberian
Copy link

Hi, this should go after previous PRs. We don't need getIndentation.js anymore.

When we are at the beginning of line at our code-block we can remove it by making attempt to go back to previous indent. Here we are checking the current indent and if it zero and this is the first line of the code-block, we do nothing.

@newsiberian newsiberian changed the title Protection against removing current code-block Protection against current code-block removing Dec 7, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-12.2%) to 87.075% when pulling 6d75d13 on newsiberian:protect-against-code-block-removing into c219dc5 on SamyPesse:master.

Copy link
Collaborator

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Can you add some tests that show the broken behavior and what this fixes please? Thanks!

@mxstbr
Copy link
Collaborator

mxstbr commented Dec 7, 2017

If I understand this correctly, when you're in an empty code block with the cursor at the first line, i.e. like so: (where | is the cursor position)

|

Rather than trying to remove the (non-existant) indentation we just let DraftJS handle it instead? What's not working about this right now?

@newsiberian
Copy link
Author

You can try it here with backspace
If you try to put cursor into position you mentioned (even if text presents in code-block) and press backspace it will remove code-block.

@mxstbr
Copy link
Collaborator

mxstbr commented Dec 7, 2017

Ohhh I see, that's annoying, great catch! Let's get some tests in here to make sure this doesn't happen again accidentally, and then let's ship it 🚀

@newsiberian
Copy link
Author

I'll try to add tests tomorrow

@mxstbr
Copy link
Collaborator

mxstbr commented Dec 7, 2017

Awesome, thank you! There's a bunch of tests already, you can just model them after that 👌

- fixed previous commit changes;
- added test;
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 98.701% when pulling d4796f7 on newsiberian:protect-against-code-block-removing into c219dc5 on SamyPesse:master.

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

3 participants