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

Reconsider how removeFormat works #954

Open
jhchen opened this issue Sep 12, 2016 · 6 comments
Open

Reconsider how removeFormat works #954

jhchen opened this issue Sep 12, 2016 · 6 comments

Comments

@jhchen
Copy link
Member

jhchen commented Sep 12, 2016

The current removeFormat behavior might be unintuitive and might be worth reconsidering. It removes all inline formats within the given range and all block formats on lines the range is on. So if you have (example 1):

<h1>He|y</h1>
<h2>Wo|rld</h2>

and your selection surrounds the "|" characters, { index: 2, length: 6 } to be precise including the "|" characters, removeFormat(2, 6); would remove both header formats. If your cursor position was (example 2):

<h1>H|e|y world!</h1>

removeFormat(1, 3) would also remove the header.

A few plausible behaviors for removeFormat() include:

  1. Remove all block formats on lines included in range (current behavior)
  2. Remove all block formats on lines included in range, if range spans more than one line
  3. Remove block formats only if line's newline character is within given range.
  4. removeFormat should not handle block formats at all.

Google Docs seems to take option 4's approach. There is also the possibility of having the clean toolbar button do something different than the removeFormat API. Perhaps some of the addition logic of single line vs multiple line could go into the clean toolbar handler (and thus customizable by the user) and the API can be more straightforward.

Thoughts?

@jhchen
Copy link
Member Author

jhchen commented Sep 12, 2016

@cchimi
Copy link

cchimi commented Sep 12, 2016

Thanks for taking this up. I would personally vote for 2; if you're removing block formats I think it should be expected that you would remove all block formats touched by a selection. Alternatively you could have separate functions for removing line formats and inline formats, and let the code determine what to do based on the selection. I'm new to Quill and not familiar enough to guess at which is more in keeping with your style.

In the meantime, though, you may want to edit the documentation which says:

Line formatting will be removed if the end of the line is included in the range.

That would make it clearer that the function currently doesn't distinguish scenarios based on the presence of a newline.

@cchimi
Copy link

cchimi commented Sep 22, 2016

Not sure if this is helpful, but this is what I'm currently doing with my Clear Format button:

function clearFormats(thisQuill, range){
  //Check for newline in selection.
  var eolInd = thisQuill.getText(range).search(/\n/);
  if (eolInd > -1){
    //If the newline is found and is at the end of the selection,
    //remove block formats up to the second to last character.
    //This prevents the formatting of the next line from being
    //removed. Otherwise, remove block formats for the range.
    if (eolInd + 1 == range.length){
      thisQuill.removeFormat(range.index, range.length - 1);
    }else{
      thisQuill.removeFormat(range.index, range.length);
    }
  //If a newline isn't found but the selection is collapsed and
  //directly precedes a newline, remove the format up to the 
  //character preceding the cursor. Should probably be cleaned up.
  }else if (range.length == 0 && thisQuill.getText(range.index, 1).search(/\n/) > -1){
    thisQuill.removeFormat(range.index - 1, 1);
  }
  //Grabs character formats from a separate object; would be handled
  //differently in a different implementation. 
  var clearChars = {};
  for (var k in charNames){
    if (charNames.hasOwnProperty(k) && charNames[k].length > 0){
      clearChars[k] = false;
    }
  }
  //Removes inline character formats from the range. This may not
  //be necessary generally, but the way in which I'm using Quill 
  //means that removeFormat doesn't touch the inline formats.
  thisQuill.formatText(range.index, range.length, clearChars);
}

I think there may be some quirks related to how I have this set up that mean removeFormat works differently for me, so the inline format removal might not be required in a different implementation. This approach is working for me so far although it needs some more testing.

@benbro
Copy link
Contributor

benbro commented May 14, 2017

Screen capture of 'Clear all Formatting' button in Word on Windows 7. It uses option 1.

  • A click removes block formatting of any line included in the range, even if the range is collapsed.
  • A click removes inline formatting included in the range.
  • If the range is collapsed, a click removes inline formatting of a word around the cursor.

I think that the first two points are the expected behavior and what I would like to see in Quill. I'm not sure about the third point.

I like option 1 which is the current behavior.
What I'm only missing is removing block format when the range is collapsed #1228.

@pedrosanta
Copy link

pedrosanta commented Nov 13, 2017

Hum, nice question question/discussion.

Honestly, I think the current behaviour (1) seems to be the more faithful to what the method proposes. I feel that behaviour to be the most unambiguous one for that particular method - perhaps a better wording could be removeAllFormats, idk.

On another hand, variants for the other behaviours could also be considered - I was thinking of something along the lines of removeInlineFormats, removeBlockFormats and, hell why not, removeEmbeds, along the above one. Would that make sense?

Another thing I got into (but still have to confirm that one, I'm afraid bundling may be causing an issue on non-allowed formats removal by Parchment) was the need to remove only specific formats (a list of them) for a given range - including embeds. (Could be an option for these methods?) Thoughts?

@ArsalanSavand
Copy link

Any updates on this?

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

No branches or pull requests

5 participants