guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

There is no underline command? #306

Closed koistya closed 9 years ago

koistya commented 9 years ago

It seems like underline command is not there. How can I add one?

hmgibson23 commented 9 years ago

Hi, you can add one pretty easily either by using the https://github.com/guardian/scribe/blob/master/src/api/command-patch.js or the https://github.com/guardian/scribe/blob/master/src/api/command.js. Use the patch if only want to patch (as in not alter that much) a command and the command if you'd like to do some more heavy lifting with it.

Here is an example of a command: https://github.com/guardian/scribe/blob/master/src/plugins/core/commands/indent.js

and here is an example of a patch: https://github.com/guardian/scribe/blob/master/src/plugins/core/patches/commands/bold.js

All patches and commands in the core plugins are just plugins, so there could be a separate project for underline command. Although I think it probably belongs inside the main project.

JeSuis commented 9 years ago

@koistya Underline will work by default just add the button.

<button type="button" title="Underline" data-command-name="underline">
rrees commented 9 years ago

@JeSuis @koistya I gave the underline command name a go and it didn't work by default because underlining is a styling thing rather than an HTML element.

I suspect we need to create a command that wrapped the selection in a span with a standard class that could be targeted by CSS.

robinedman commented 9 years ago

@rrees I'd argue for not doing such a thing. At least not in scribe core. I mean in a plugin would be totally fine. But what we'd do is introduce more presentational markup at the expense of keeping it semantic. We do already use bold instead of strong, and italics instead of emhpasis but I don't think that's a compelling reason to continue too far down that path.

@koistya Another approach you could take: One solution that would work today is to use scribe-plugin-formatter-html-ensure-semantic-elements which converts <I> to <EM> and <B> to <STRONG>. You can then style them the way you want to. If you need underline, italics and strong then creating a plugin implementing @rrees's solution is better though.

rrees commented 9 years ago

@robinedman ah, sorry, yes I see my answer was ambiguous, I meant create a plugin that does this to avoid many people all writing their own version of the plugin. If we could agree what the structure was then the plugin is probably simple.

JeSuis commented 9 years ago

@rrees what browser are you using? I have to cross check but it is working for me just using the command name, I'll see if I can get a stand alone demo later to make sure nothing else is interfering.

For custom stuff I made a command that wraps the selection in a span then adds a class. But I need a better way so I don't have a ton of these plugins in the case I have multiple custom stylings.

rrees commented 9 years ago

@JeSuis FF37

JeSuis commented 9 years ago

@rrees

My mac air says FF35 is the latest, although I think my mac book pro at home has 37, but here's a screen of underline working. I'll see if I can find some time to put into a fiddle today.

image

JeSuis commented 9 years ago

@rrees @koistya didn't get it working in a fiddle but my stand alone demo works. I uploaded it here, didn't get to setup a demo yet, but you can check the files out. I tried to remove all custom stuff. Its weird that one works and not the other so maybe I did something that got it working and didn't realize it.

https://github.com/JeSuis/scribe-playground

rrees commented 9 years ago

@JeSuis I'm starting to think this might because we actually remove the underline tags with the sanitizer plugin.

JeSuis commented 9 years ago

@rrees you are correct, my demo has it in the whitelist, the fiddle didn't, I just tried it in the fiddle and it worked.

rrees commented 9 years ago

@koistya So the short answer to your question is yes! I have updated the demo page with an example and added an entry to the FAQ

Thank you @JeSuis for helping figure this out.

koistya commented 9 years ago

@rrees thanks!