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

Fix for indentation behaviour #32

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

Conversation

newsiberian
Copy link

Hi, fix for #31
Also added new argument for indentation depth for onTabs, handleReturn;

- added new argument `indent depth` for onTabs, handleReturn;
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 97.917% when pulling bb682a4 on newsiberian:fix-31 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.

Wow, what an awesome patch! I'll fully review this later today, mind removing that version number upgrade for now? Thanks!

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "draft-js-code",
"version": "0.3.0",
"version": "0.4.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please undo this change

Copy link
Author

Choose a reason for hiding this comment

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

ok, sorry for that

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 97.917% when pulling 9928552 on newsiberian:fix-31 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.

I don't 100% understand the issue here, what is the behavior you're experiencing right now and what is the behavior you want to happen instead?

@@ -105,7 +105,7 @@ class Editor extends React.Component {
const { editorState } = this.state;
if (!CodeUtils.hasSelectionInBlock(editorState)) return 'not-handled';

this.onChange(CodeUtils.onTab(evt, editorState));
this.onChange(CodeUtils.onTab(evt, editorState, 2));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably shouldn't be hard coded? Rather than being provided as an argument it should ideally be detected—that's what the getIndentation.js utils is for afaict.

Can we somehow infer this instead of hardcoding it? Why do we need this argument here?

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm. it's not hardcoded. You can set it with props if you want, or you mean we should show something like CodeUtils.onTab(evt, editorState, indentDepth) here?

And again. I don't how getIndentation can help here. The case I see for it is to detect current line indentation and set next line similar indentation on that data, like insertNewLine does

@newsiberian
Copy link
Author

If you try to press TAB here several times it will indent on 4, then 4, then 8 and 16 spaces, etc. I'm don't understand the logic behind getIndentation

@mxstbr
Copy link
Collaborator

mxstbr commented Dec 7, 2017

The logic behind getIndentation is that not everybody indents with two spaces.

Imagine code like this, | is the cursor position:

function bla2(x) {
  const a = "b";
  |
}

When a user presses tab now, this is what should happen:

function bla2(x) {
  const a = "b";
    |
}

The cursor is indented by two spaces, because they use two spaces for indentation.

Now imagine this code:

function bla4(y) {
    const b = "c";
    |
}

What happens now when a user pressed tab? (note the four spaces indentation) Ideally it indents by four spaces, since the user uses four spaces for indentation:

function bla4(y) {
    const b = "c";
        |
}

I think that's what the getIndentation function was for, detecting that? Maybe? (I wasn't the original author of the package so not 100% on the original intentions) Anyway, that's the way it should work but by hardcoding the 2 there we can't make that work... 😕

@newsiberian
Copy link
Author

Ok, again, if I understand you correctly we could have the following:
Let's say the default is 4 spaces
case 1:

function () {
    | // <-- in that case we could use default or preset indent

case 2:

function () {
  var x = 1;
  | // <-- we are looking on previous line with 2 spaces and use
// this 2 spaces-indent for the current line

case 3: at the same time

function () {
    var y = 2;
    | // <-- and again here (even if several line above we were using 2
// spaces indent) we are looking on previous line indent and using it for this line

Is this correct?

@mxstbr
Copy link
Collaborator

mxstbr commented Dec 7, 2017

Yep, we just choose some default and then try and smartly guess the indentation preferences of the user based on their previous code in the code block! So rather than the user saying "I want everybody to type with 2 or 4 spaces indentation" we just take care of it. That'd be ideal, right?

@newsiberian
Copy link
Author

ok, now I get it, but can we leave new argument? because now we have only 4 spaces by default, which is not cool, so we will have something like:

var DEFAULT_INDENTATION = '    ';

function getIndentation(text, indent) {
  var result = detectIndent(text);
  return result.indent || indent || DEFAULT_INDENTATION;
}

@mxstbr
Copy link
Collaborator

mxstbr commented Dec 7, 2017

Let's just make 2 the default, I prefer that anyway, and then infer from the user if possible.

@andria-dev
Copy link

andria-dev commented Mar 23, 2019

@mxstbr @SamyPesse is there any way we could merge this? Or should I open up a PR that allows a set indentation size while defaulting to inferring the correct indent size?

@newsiberian
Copy link
Author

@ChrisBrownie55, hi, check my fork. It is published and probably has a fix for that use case

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