Skip to content

Added support for shift+tab to decrease indents. #36

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

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

Conversation

cbeninati
Copy link

No description provided.

@coveralls
Copy link

coveralls commented Jan 28, 2018

Coverage Status

Coverage decreased (-3.6%) to 95.714% when pulling b21e242 on cbeninati:Support-Shift+Tab-to-remove-tabs-#6 into c219dc5 on SamyPesse:master.

@mxstbr
Copy link
Collaborator

mxstbr commented Jan 28, 2018

How does this compare to #30?

@cbeninati
Copy link
Author

cbeninati commented Jan 28, 2018

It works more like @newsiberian mentioned regarding WebStorm's functionality.

Hi, here we should some extra work. I've checked again WebStorm behaviour on shift+Tab and backspace and their are differ.

This adds indentation decrease, which is a line-based action. That meansit will remove indentation from the start of the line regardless of the cursor's position.

If there is no indentation at the start of the line, it will ignore the request to decrease indentation.

For my needs, and this seems to be reflected in @newsiberian's comment above, there should separate functionality for 'backspace' (remove from the cursor's position) and 'shift+tab' (remove the line's indentation).

If we wanted, to add more flexibility, we could add an optional parameter in onTab() to specify the intent. Something like:
onTab(event, editorState, action) In which action could be 'backspace' or 'decrease indent' or something like that.

@mxstbr
Copy link
Collaborator

mxstbr commented Jan 28, 2018

I see, that makes sense!

This breaks for me on any code block that has > 1 line of code, see this recording: https://www.dropbox.com/s/qspu16vnb2zrrjr/remove-tab-bug.mov?dl=0

Mind fixing that and adding some tests verifying both that case and the "no indentation at start" case work as expected? Thanks!

@cbeninati
Copy link
Author

Yeah, no problem, thanks for catching that!
I'll look into it either later tonight or tomorrow.

When handling tab decrease, we shouldn't assume the user wants to remove all indentation at the start of the line. With a new 'default tab' setting, we can define the tab setting (number of spaces) for the editor. Now, when a user wants to decrease their indentation, it will do it one tab at a time.
@cbeninati
Copy link
Author

cbeninati commented Jan 30, 2018

Ok @mxstbr, I fixed the multiline issue. In doing so, I also added support for 'default tab size' to be optionally passed through to the onTab() method as a third parameter.

This allows for an editor to have it's own declared tab spacing setting instead of just reading the indentation spacing (although that is a fallback if the tabSize is set to null).

This is important because, with knowledge of an individual tab size, we can decrease indentation one tab at a time instead of just removing all spaces at the beginning of the line.

I'm glad to discuss alternative implementations also, but I needed something in place in order to handle true tab-by-tab decreasing.

If you agree with my implementation, then I'd be happy to update the documentation also, but I wanted to give you a chance to review it first.

@cbeninati
Copy link
Author

@mxstbr Is there anything I can update or issues I can fix or would you like to discuss alternative approaches to help resolve this?

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.

Oh sorry, I totally missed this. As long as the explicit value passed to onTab is optional/a fallback and we still infer tab size based on the rest of the code block I'm happy with this change and would love an update to the docs!

@cbeninati
Copy link
Author

Ok, sounds good. I'll update the docs and let you know when it's ready.

@wrahman0
Copy link

ETA when this will merge?

@mxstbr
Copy link
Collaborator

mxstbr commented Mar 21, 2018

Whenever @cbeninati adds stuff to the docs!

- also added a new test for Shift+Tab support of custom tab sizes
- some code cleanup
- some miscellaneous documentation cleanup for consistency
@cbeninati
Copy link
Author

@mxstbr Apologies for the delay. I diverted from the DraftJS project I was using this for.
The README has been updated to reflect the new onTab functionality. Not much to it. :)

@cbeninati
Copy link
Author

@mxstbr Let me know if anything's missing from the documentation. There wasn't much to do.

@mxstbr
Copy link
Collaborator

mxstbr commented Mar 30, 2018

It doesn't seem to detect tab size based on the rest of the code?

Recording of broken shift+tab behavior

Note how pressing "Backspace" does the right thing (removes the indentation) but pressing shift+tab doesn't do anything.

@pReya
Copy link

pReya commented Mar 1, 2019

@cbeninati Any chance you can fix the problem described by @mxstbr ? I'd really appreciate this getting merged. Thanks for all your work.

@RafaelZasas
Copy link

Will this be merged soon? Would love to have Shift Tab functionality

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.

6 participants