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

Better integration between formatting bar and Scribe, switchable formatting options. #395

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

Conversation

jamesthurley
Copy link
Contributor

I realise I haven't asked anyone if these changes are required or even consistent with your vision, but I needed them for my project and so I tried to implement them in a way you might approve of.

Essentially I needed more formatting options as I'm looking to use Sir Trevor to replace Medium-Editor on my site. Scribe provides what I need in terms of formatting, so I just needed to update the formatting bar and scribe plugins.

However in anticipation of creating a PR, I wanted to do this in a way that it was easily configurable for those that didn't want the extra formatting options, or a subset (e.g. #40), and that involved some minor refactoring. I also realised not all the formatting options would be appropriate for every block type, so I needed to make them switchable on a block-by-block basis.

Ideally I wanted one place where I could say "I need strikethrough", and the formatting bar, tag sanitising, and keyboard shortcuts would all update. Similarly, the heading block should be able to say "I don't support bold", and the bold option, keyboard shortcut and tag sanitising would also update to remove support there.

The Changes

The first thing I realised was that Scribe already has a plugin to handle keyboard shortcuts for commands, so I got rid of the custom handing in block.js and used Scribe's instead. I also got rid of the formattable property, as it was a bit broken in the current implementation (see #147) and I was going to solve that issue elsewhere.

With a bigger formatting bar it was partially disappearing off the left edge of the page when I selected the first word, so I updated calculatePosition so that it behaves in the same way as the medium-editor project, where it hugs the left side instead of disappearing.

I updated scribe-interface so that it takes a simple formatting configuration:

textFormatting: {
  bold: true,
  italic: true,
  underline: false,
  // etc
}

That configuration is overridable by the user and by each block, in the same way as existing options are. The scribe-interface then sets up the scribe plugins, the tag sanitizing, the keyboard shortcuts and the formatting bar options depending on the requested configuration. Also if the block implements a configureScribe function, that function is now passed all the required data to extend what scribe-interface is now doing.

To link it to the formatting bar, the initScribeInterface method returns a structure with both the scribe instance and the formatting bar options. The block (and multi-editable) saves those to the block.

I then updated the formatting bar to use that data. If the options have changed because you've moved to a different block, it re-builds the formatting bar (see the updateButtons method). If there are no commands to display (all formatting options are disabled), it does not display the formatting bar (this indirectly solves #147 as all keyboard shortcuts would also be disabled).

I updated the default blocks to remove formatting options that didn't make sense.. for example removing bold from the heading block (although I understand you might want to remove all formatting from the heading block, which you can also now do).

I updated the formatting e2e test to check the new formatting options.

Finally, I set the default options for text formatting in config.js. I've set this so that by default the formatting options are identical to before this change. I've changed the example.html to have everything enabled however, in case people want to play.

The Controversial Bit

As part of the new formatting options, you can enable (gulp, bare with me!) headings and lists and quotes. I totally realise this is not your philosophy with ST.

But I'll tell you why... Like I said, I was looking for replacement for Medium-Editor. Medium-Editor is great, but the support for embedding images and videos is not there out of the box, and the plugin they recommend is extremely bloated. I expected that implementing it myself in pure contenteditable would be a PITA.

So I was looking at Sir Trevor, and really liked what I saw... adding images or anything else was intuitive, you could drag-drop them in a sensible way, you can add metadata such as alignment if necessary, and it's written to be really easily extended. Fantastic.

BUT, I saw it uses blocks for things such as headings and lists. Personally I felt this was unintuitive for the user, it created extra work for them (it interrupts the writing flow to click a series of buttons), and it's quite different to how other editors do things. I didn't feel for my website I was gaining anything with this different approach, but the UX did worry me.

This put me off using ST for a while, until I realised I could get the best of both worlds doing the changes in this PR, and removing blocks like heading and list. Users get the writing experience they are used to, but have all the flexibility of blocks when inserting other more blocky things.

However I totally get that is just my use case and my opinion. But I at least wanted to explain why I added those formatting options despite them also being available as blocks. If you accept this PR I hope you keep those as I expect other people might appreciate having the option. If you enable lists it even adds the "smart lists" scribe plugin, which allows you to start a list by typing "1. ", or "- ". It's a very slick writing experience :)

Ok, sorry this was so long. Let me know what you think.

@SgtOddball
Copy link
Contributor

Hi @jamesthurley, I'd be interested in looking further into this as I've managed to add extra formatting to the scribe bar myself (though only the default scribe ones, not had the time to dig through and figure out how to add a custom plugin) without changing the core of Sir Trev.

But from what I've seen you've gone beyond what I did (See #333 but that's adding the whole scribe option to the initialisation config rather than having a simple trigger) and you've sorted the format bar bleeding off the edge of the screen (not so much of an issue for me due to padding) which is a nice touch too (I got lazy I'll admit). - tell a lie, I had made a change for it to fix if the block was being moved left or right (say by padding for a menu) thought slightly different from yours. Will test and see which works better.

I'll have a look over and keep you posted.

@jamesthurley
Copy link
Contributor Author

Hi @SgtOddball, thanks for the feedback! Based on what was going on in #333 I realised I had removed a point of extensibility in my changes. Therefore I've added an extension point to more easily extend scribe from the configuration, and updated the example. So to add a "superscript" item to the formatting menu I initialised ST like this:

      window.editor = new SirTrevor.Editor({
        el: $('.sir-trevor'),
        blockTypes: [
          "Heading",
          "Text",
          "List",
          "Quote",
          "Image",
          "Video",
          "Tweet"
        ],
        textFormatting: {
          bold: true,
          italic: true,
          underline: true,
          strikethrough: true,
          link: true,
          h1: true,
          h2: true,
          list: true,
          blockquote: true,
          configure: function(scribe, scribeConfiguration){
              scribeConfiguration.formatBarCommands.push({
                  name: "Superscript",
                  cmd: "superscript",
                  text: "^",
                  title: "superscript"
              });
              scribeConfiguration.sanitize.sup = {};
              scribeConfiguration.shortcuts.superscript = function (event) {
                  return (event.metaKey || event.ctrlKey) && event.keyCode === 80; // p
              };
          }
        }
      });

So in the textFormatting.configure section I've added the item to the format bar, updated the sanitisation to allow <sup> tags, and created a Cmd+P/Ctrl+P shortcut.

screenshot 2015-10-20 11 00 37

I tried it with justifyRight but the command didn't seem to do anything, even when I disabled the Scribe sanitizer plugin. But I'm not very familiar with contenteditable, so maybe someone with more knowledge can help there...

Haven't had a chance to look at the body padding issue yet.

@SgtOddball
Copy link
Contributor

Humm, for justify, I just used the following

{
  name: "justifyLeft",
  title: "justifyleft",
  cmd: "justifyLeft",
  text: ""
},
{
  name: "justifyCenter",
  title: "justifycenter",
  cmd: "justifyCenter",
  text: ""
},
{
  name: "justifyRight",
  title: "justifyright",
  cmd: "justifyRight",
  text: ""
},
{
  name: "justifyFull",
  title: "justifyfull",
  cmd: "justifyFull",
  text: ""
}

For the icons I used glyphicons since I'm using Sir trev on a bootstrap layout so I've got some custom CSS along the lines of

.st-format-btn--justifyLeft:before {
    content: "\e052";
    display: block;
    font-family: 'Glyphicons Halflings';
    -webkit-font-smoothing: antialiased;
    -moz-osx-font-smoothing: grayscale;
}
.st-format-btn--justifyCenter:before  {
    content: "\e053";
    display: block;
    font-family: 'Glyphicons Halflings';
    -webkit-font-smoothing: antialiased;
    -moz-osx-font-smoothing: grayscale;
}
.st-format-btn--justifyRight:before  {
  content: "\e054";
  display: block;
  font-family: 'Glyphicons Halflings';
  -webkit-font-smoothing: antialiased;
  -moz-osx-font-smoothing: grayscale;
}
.st-format-btn--justifyFull:before  {
  content: "\e055";
  display: block;
  font-family: 'Glyphicons Halflings';    
  -webkit-font-smoothing: antialiased;
  -moz-osx-font-smoothing: grayscale;
}
.st-format-btn--Bold {
  font-weight: 700; 
}
.st-format-btn--Underline {
  text-decoration: underline; 
}
.st-format-btn--superscript:before{
  content: "\e255";
  display: block;
  font-family: 'Glyphicons Halflings';    
  -webkit-font-smoothing: antialiased;
  -moz-osx-font-smoothing: grayscale;
}
.st-format-btn--subscript:before {
  content: "\e256";
  display: block;
  font-family: 'Glyphicons Halflings';    
  -webkit-font-smoothing: antialiased;
  -moz-osx-font-smoothing: grayscale;
}

@SgtOddball
Copy link
Contributor

Also interesting to note that you've added sanitisation for <sup> etc. I've not added it to my own version and it's worked out of the box with no complaints or strange behaviours.

@jamesthurley
Copy link
Contributor Author

I see the sanitizer was only added to ST on August 13th (3d6ddad)... are you on that version?

@SgtOddball
Copy link
Contributor

That'd be it, I'm still on 0.5.0-beta3 ... urgh... I'll have to have a looksee at merging those changes with my own repo when I get chance. Joy...

On the plus side, I now know of at least one potential issue going forwards.

On a related note, have you tried the justify values yet?

@jamesthurley
Copy link
Contributor Author

I tried pasting in your justify values, but they aren't working for me even if I disable all Scribe plugins. I also tried it on a fresh pull of 5.0.0 and it didn't work there either, so I don't think the problem is anything in this pull request. It would be interesting to know if they work for you after merging in 0.5.0.

I also tried adding padding: 300px; to the body element in the ST example index.html, and my toolbar was still aligning ok:

screenshot 2015-10-21 11 32 43

Can you reproduce your issue in the ST example?

@SgtOddball
Copy link
Contributor

@jamesthurley The padding issue is more a case of if you have some other element on the left thats z-indexed over the top of the main content. You'll then find the edge of the toolbar being obscured by the element.

I will admit it's something of an edge case but I've found it helpful to cover for it.

I've checked over the new parts from the latest bower release and it still seems to work fine, might sync the repo at home and see if there's anything further the bower files don't have (still not got the hang of getting repos to compile right though npm, always end up missing something or just having it fail but i'll try again when i've got some time).

@timmyg
Copy link

timmyg commented Dec 9, 2015

Having trouble following this one, is it possible to add additional underline, h2, etc. to FormatBar now? Also, whats adding align left/center/right look like (to the controlBar)?

@SgtOddball
Copy link
Contributor

It's always been possible to add additional functions to the Format Bar, it was just a case of understanding what the underlying scribe tool needs so that you can add them in.

As already listed above it's just a case of adding in references in your initial Sir Trev call as part of it's configuration.

@jamesthurley
Copy link
Contributor Author

Hi @timmyg, what I basically found was that to add a new item to the format bar you potentially need to do some or all of the following:

  • Add a format bar icon.
  • Adjust Scribe sanitisation options.
  • Add Scribe plugins.

You could already adjust the format bar in configuration, but if the Scribe sanitisation and plugins weren't adequate you had to fork and edit the code appropriate code. I wanted to make it so that it could all be done in one place, in configuration, without needing to fork.

In addition I found:

  • ST was duplicating scribe functionality by handling keyboard shortcuts, so I removed that duplication and just let scribe handle it.
  • ST currently displays the same formatting options for all block types, so I added the ability for some block types to remove options that didn't make sense for them (e.g. the bold button does nothing in a heading block). You can see this in heading.js, quote.js and list.js.
  • Configuring certain buttons was fairly complex, so I created some defaults and made them toggleable in configuration. You can see these in scribe-interface.js.

You can see an example now of adding a custom formatting option and toggling default options in examples\index.html. The custom option it adds is superscript, which didn't require an additional scribe plugin. But if it had, the plugin could also have been added there.

@raffij
Copy link
Contributor

raffij commented Dec 9, 2015

@jamesthurley Thanks for this pr. Been thinking about it for a while and how best to integrate. I'll feedback on this later today.

@jamesthurley
Copy link
Contributor Author

Thanks @raffij, I appreciate it's a lot of changes for a PR so I'd understand if there are reasons for it not being integrated.

@timmyg
Copy link

timmyg commented Dec 9, 2015

@raffij @jamesthurley @SgtOddball Thank you! I now have the additional formatting options available, but what about the left/center/right align (in the bottom left corner). Does anyone have an example of this (recent usage of controllable, dont mind making own block type)? I have recently forked and created a working Scribe plugin but am still getting used to the landscape, but dont have an issue editing source code. Thanks!!

@jamesthurley
Copy link
Contributor Author

@timmyg I tried adding left/center/right align buttons but couldn't get them to work, even with no Scribe sanitization and without the changes in this PR. Not sure what is issue is, but it doesn't seem to be related to this PR. I didn't need those buttons so didn't look into it any further.

@SgtOddball shows how he did got it working in the above comments, but he was using an earlier version of ST and I couldn't I get his buttons to work in the latest version.

@timmyg
Copy link

timmyg commented Dec 9, 2015

Yea doesnt seem to work like it did in previous versions in any capacity.

I tried the below and it seems to create the html but opacity is 0 on .st-block__control-ui...
TeamLifecycle@a7e9b01

Going to work on making it look pretty

@raffij
Copy link
Contributor

raffij commented Dec 9, 2015

@timmyg I can take a look, but not got time today.

@timmyg
Copy link

timmyg commented Dec 9, 2015

@raffij cool thanks... yea i think it's just an issue with the new css, trying to get it back to how she looked before.

@timmyg
Copy link

timmyg commented Dec 9, 2015

you can see quick fix to get it to show up again...
TeamLifecycle@7d346d4

@SgtOddball
Copy link
Contributor

I'll double check I've got the latest version (0.5.0) but I've had no problems with my code on it so far (I did this initially on 0.5.0 beta 3 but the changes didn't seem to affect the justifying, but I'll recheck).

  • Have rechecked.... ahhh I might need to spend some time on this, it's changed a tad from the version I've got (just seen the builds @raffij did and everythings changed, or at least it seems that way)

@SgtOddball
Copy link
Contributor

On a related note, the link on the project page for Sir Trev still lists 0.4.0 as the latest release from the zipball/master link. (just saying...)

@SgtOddball SgtOddball mentioned this pull request Feb 15, 2016
raffij pushed a commit that referenced this pull request Mar 1, 2016
@raffij raffij added this to the v1.0 milestone Jul 11, 2016
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.

4 participants