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

Heading support #532

Closed
wants to merge 17 commits into from
Closed

Heading support #532

wants to merge 17 commits into from

Conversation

yadutaf
Copy link

@yadutaf yadutaf commented Mar 2, 2012

Here is a preliminary but fully functional implementation of title support for etherpad-lite.

  • titles h1->h4 (arbitrary)
  • HTML export is functional
  • dropdown to choose title
  • style following title is regular text

This implementation relies a lot on the list infrastructure that I know quite well. My next project is to slowly refactor it to to ease the addition of new formating features.

I also added a few comments blocks in the code to help ou new contributors understanding the architecture.

Hope you enjoy it :)

EDIT: I have no idea why github indicates that the code is 3 month old. I created the branch yesterday ! It should be easier to merge than ordered list support.

@JohnMcLear
Copy link
Member

Will play w/ this now while I have 5mins..

@yadutaf
Copy link
Author

yadutaf commented Mar 2, 2012

Cool !

Just forgot to say that I have a running demo here: http://notes.jtlebi.fr:8080/p/test

@JohnMcLear
Copy link
Member

This should really be a plugin (Sorry I know that's a pain)

The lines go out of vertical sync as you change font size -> see: http://twitpic.com/8r39gu

It might make more sense to provide a (h1), (h2) button and leave it at that? OR A+ a- style buttons for "make font bigger/smaller"?

The box around the current drop down has some weird styling attributes (changing to (c) would fix this)

Selecting a whole line applies the attribute to the next line, replicate by clicking 3 times on a line of text then changing the title type.

This pull request shouldn't really be called "title support", it should be called "heading support".. Title support is something completely different.

Lots for you to get on with there but great work so far, I'm sure with my feedback you will get this finished and frequently adopted. It's worth saying early on that I'm notoriously difficult to please when it comes to adding new functionality to the front end so apologies if you got a lot of unexpected bug reports.

@yadutaf
Copy link
Author

yadutaf commented Mar 2, 2012

I definitively agree that it should be a plugin. Sadly the plugin infrastructure is not yet available and it will require quite a lot of time for me to learn it. My idea was to do some code cleanups (DRY) in the formating code once merged. I like to work step by steps instead of committing bulks of 10,000 lines at once !

I wanted to add support for text styling as GDoc does and I started with headings. This is why I choosed to do a dropdown instead of multiples buttons. Maybe later someone will add pre-formated text support in the same place.

What do you mean by vertical sync ? Yes the lines gets bigger when the font is bigger. No idea how to do it another way :)
EDIT: understood it. I will work on it
EDIT2: when moving the font to "normal", it becomes obvious that this is because of empty lines... Nonetheless, this is still an important issue to me.

This is heading support, not font size trick. I wanted it to be semantical. As a use case, I take notes during my lectures using Etherpad-lite. Instead of created pads lectureA-1, lectureA-2, ...,I added titles to distinguish the different sections of the lecture. Someday, I wish I could add folding along with it to focus on the part I type while keeping an eye on the outline. This could not be easily achieved by doing a trivial A+, A- button. Anyway, Etherpad is not a WYSIWYG word-processor. It sounds like a nonsense to add font size tricks.
Btw, the dropdown obviously need to be reworked.

I can not reproduce the bug you describe in text selection + heading application. Since this is handled by the same code as lists, you should have it with ordered and unordered lists too. I'm using Firefox on Linux. I think it should be filled as a separated bug bug report don't you ?

Sorry for the title/heading mismatch. English is not my native language. In French "headings" are "Titres" which is close from "title"...

I will try to rework the dropdown ASAP. Do you know any mean of getting notified when the text selection changes and to read it's attributes ? It would allow to keep the selected item in the dropdown in sync with the active text and thus to enhance the user experience a lot.

What is your opinion about it ?

Jean-Tiare Le Bigot added 2 commits March 2, 2012 21:02
…he header DOM generation that caused breakage in the authorship highlighting code
@yadutaf
Copy link
Author

yadutaf commented Mar 3, 2012

Hi !

@johnyma22 I have to admit that I was a bit frustrated by your answer but I had a very good time getting live feedback from @0ip yesterday directly on the pad. It's really amazing to contribute for a real-time collaborative text-editor this way :)

We worked on small bug fixes and big CSS improvments for the toolbar. I'm pretty sure you will like it as they look much, much more professional and user friendly !

To make it short :

  • much better styling in the toolbar
  • dropdown behaves like a sub-menu
  • author coloration fix (typo domline.js)
  • vertical sync should be OK now for line numbers

In my opinion this is now ready. Any feedback ?

cheers,

@JohnMcLear
Copy link
Member

Some feedback/bugs:

  • Line spacing is too tight between H2 and P: http://twitpic.com/8rfvd8
  • You can't highlight a single word and change that to H2 without it affecting the whole line
  • Dragging text that is h2 into a p creates a new line break then when dragged back out leaves the line breaks.
  • Indenting H1 or H2 loses all styling (goes back to P)

Sorry about reporting these bugs, I'm pretty good at knowing what to test for.. Apologies if my feedback was less fruitful than 0ips help. It looks great now and the functionality is getting there..

@0ip we could merge this into core once it's stable but I would strongly be minded that in "Settings" we provide a "Use Rich Text Editing" checkbox that enables this on a per user(And/or Global) basis. This checkbox may turn on "Change font size, font face and headings" I want to avoid at all costs cluttering the toolbar.

@yadutaf
Copy link
Author

yadutaf commented Mar 4, 2012

Many thanks for your precise and valuable feedback @johnyma22 . I fixed 3 of the issues you reported. In my opinion the second one is more a feature than a bug :) In my mind, Headings have a semantical value, not only a formating one.

  • fixed line spacing
  • fixed Drag & Drop. Btw, this is weird. The workaround was to consider Hn as inline in contencollector.js while still considering it as block in ace2_inner.js not to break the cursor ???
  • disabled indentation for headings as it does not make sense.

I agree that it would be nice to get an option to move to raw text/minimal text editor. Some editors are completly cluttered by their "user friendly" interface and we definitively want to avoid it. Maybe someone else could implement it in order no to mix the features between pull requests ?

@JohnMcLear
Copy link
Member

The soonest I can test and pull this is Thursday. Two reasons

a) I'm really busy at the mo
b) We're not pulling any new code until the plugin framework is finally merged

@yadutaf
Copy link
Author

yadutaf commented Mar 12, 2012

What's the state of this pull request ?

@JohnMcLear
Copy link
Member

@Pita was due to update me over the weekend but never did.. Did you manage to get this as "disabled by default" with a setting to show it?

@yadutaf
Copy link
Author

yadutaf commented Mar 12, 2012

I'll give it a try later this week.

Is it OK if the button is not shown but the titles are still rendered properly if set from another client ?

@JohnMcLear
Copy link
Member

Yep :)

-----Original Message-----
From: Jean-Tiare LE BIGOT [mailto:reply@reply.github.com]
Sent: 12 March 2012 11:05
To: John McLear
Subject: Re: [etherpad-lite] Heading support (#532)

I'll give it a try later this week.

Is it OK if the button is not shown but the titles are still rendered properly if set from another client ?


Reply to this email directly or view it on GitHub:
https://github.com/Pita/etherpad-lite/pull/532#issuecomment-4449601
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of the organisation from which this email originated. If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone. Please contact the sender if you believe you have received this email in error. This email was sent by School Email - Safe Webmail and Hosted Email for Schools

@yadutaf
Copy link
Author

yadutaf commented Mar 12, 2012

I'll give it a try later this week

Jean-Tiare LE BIGOT
Administrateur jtlebi.fr, lebigot.net

-------- Original message --------
Subject: Re: [etherpad-lite] Heading support (#532)
From: John McLear reply@reply.github.com
To: Jean-Tiare LE BIGOT admin@jtlebi.fr
CC:

@Pita was due to update me over the weekend but never did..  Did you manage to get this as "disabled by default" with a setting to show it?


Reply to this email directly or view it on GitHub:
https://github.com/Pita/etherpad-lite/pull/532#issuecomment-4449384

@yadutaf
Copy link
Author

yadutaf commented Mar 12, 2012

I had a quick look at it. I think it's pretty easy to do something
hackish but I'm afraid to introduce conflicts with pending pull requests
(buttons instead of links in the toolbar).

I suggest to do it as suggested for a long time: move the toolbar
generation to JS + conf.
The JS file would also hold a var with default values for show/hide
button. Custom values would then be saved in a local cookie as the
author.
This is also a good occasion to clean up the toolbar part of the code.
The best to coordinate myself with @redhog to integrate it with the
plugin framework.

As this is a big rework, I think that the best would be to merge this
Pull request followed by the toolbar related one and then I start
working on it.

I estimate the duration to 3 weeks of spare time work.

What is you opinion on this ?

Le lundi 12 mars 2012 à 04:06 -0700, John McLear a écrit :

Yep :)

-----Original Message-----
From: Jean-Tiare LE BIGOT [mailto:reply@reply.github.com]
Sent: 12 March 2012 11:05
To: John McLear
Subject: Re: [etherpad-lite] Heading support (#532)

I'll give it a try later this week.

Is it OK if the button is not shown but the titles are still rendered properly if set from another client ?


Reply to this email directly or view it on GitHub:
https://github.com/Pita/etherpad-lite/pull/532#issuecomment-4449601
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of the organisation from which this email originated. If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone. Please contact the sender if you believe you have received this email in error. This email was sent by School Email - Safe Webmail and Hosted Email for Schools


Reply to this email directly or view it on GitHub:
https://github.com/Pita/etherpad-lite/pull/532#issuecomment-4449627

Jean-Tiare LE BIGOT
Administrateur jtlebi.fr, lebigot.net

@redhog
Copy link

redhog commented Mar 12, 2012

I have been working a bit on an EJS-based templating system for the plugin framework. With this it would be easy to generate (parts of) the toolbar using hook calls from plugins, regardless of the html markup needed.

Also, I wonder if this whole patch couldn't be done as a plugin? Esp. check if your changes overlap with the hooks.callAll calls inside the ace, or even generates a conflict...

@JohnMcLear
Copy link
Member

I spoke to @scrummitch today and we're going to hack on this on Monday.

@jtlebi @redhog can you be about on skype/jabber etc.?

@yadutaf
Copy link
Author

yadutaf commented Mar 15, 2012

I can manage IRC/Gtalk monday but not fulltime as I'm at the university

@redhog
Copy link

redhog commented Mar 15, 2012

Hm, monday evening is fine, monday day sporadically - I can't give it much time as I'm working for clients, but I should be able to answer the odd question.

@yadutaf
Copy link
Author

yadutaf commented Mar 21, 2012

any news ?

@JohnMcLear
Copy link
Member

Still planning to introduce this as a plugin. Leaving this pull request open so we can refer to the changes whilst writing :)

@JohnMcLear
Copy link
Member

The plugin framework is now available in the develop branch, however there are no docs and it might be a bit fragile :P

@yadutaf
Copy link
Author

yadutaf commented Apr 2, 2012

Last time I looked at the plugin framework, it was not compatible with this pull request. As i tried to make it the less intrusive as possible, it is tightly integrated with the list code. While moving it to a real plugin is definitively the best option, I'm not sure the bas code in the core is ready for that.

@redhog
Copy link

redhog commented Apr 3, 2012

The code in core is definitely ready for adding heading support and
other styling things as plugins using the hooks "postAceInit",
"aceInitInnerdocbodyHead", "aceAttribsToClasses" and "aceCreateDomLine".
Please see the sketchspace (https://github.com/redhog/SketchSpace)
plugin for example usage.

If you feel that you're missing a hook in the list code specifically,
please go ahead and add one :)

On 04/03/2012 12:11 AM, Jean-Tiare LE BIGOT wrote:

Last time I looked at the plugin framework, it was not compatible with this pull request. As i tried to make it the less intrusive as possible, it is tightly integrated with the list code. While moving it to a real plugin is definitively the best option, I'm not sure the bas code in the core is ready for that.


Reply to this email directly or view it on GitHub:
https://github.com/Pita/etherpad-lite/pull/532#issuecomment-4890089

Konsulent, Fri Programvare / Free Software Consultant
Mobil: +47 - 473 44 024
Telefon: +47 - 21 53 69 00, Fax: +47 - 21 53 69 09
Adr: Nydalsveien 30A, 3. etg., 0484 Oslo
Web: www.freecode.no http://www.freecode.no
http://www.freecode.no

VARNING!

E-post och ALL Internettrafik till och från Sverige, eller som passerar servrar i Sverige, avlyssnas av Försvarets Radioanstalt, FRA. (Text från Journalistförbundet)
WARNING!

E-mail and ALL Internet Communications to and from Sweden, or via servers in Sweden, is monitored by the National Defence Radio Establishment. (Text from Journalistförbundet)

@yadutaf
Copy link
Author

yadutaf commented Apr 3, 2012

thanks @redhog :)

For now on, I close this Pull request. I'll try to start over soon from a clean branch to preserve a meaningful history.

@yadutaf yadutaf closed this Apr 3, 2012
@redhog
Copy link

redhog commented Apr 3, 2012

@jtlebi Note that the SketchSpace plugin is still very beta. It is good as example code for the hooks, and it does work, but installation is rather... magic :( For now, here are some tips:

Clone the repo to $YOURETHERPADROOT/available_plugins/ep_sketchspace
npm link available_plugins/ep_sketchspace
cd available_plugins/ep_sketchspace; ./build.sh

@yadutaf
Copy link
Author

yadutaf commented Apr 4, 2012

I'll give it a try next week. In the mean time, Can you confirm me that the only mandatory files are package.json and ep.json ? Then, appart from the npm stuffs, there is no other steps like registering to do (yet) ?

Thanks !

@fourplusone
Copy link
Contributor

@jtlebi Yes, the only required files are ep.json and package.json.

You might want to have a look at
https://github.com/johnyma22/etherpad-lite/tree/disable_change_author_name_plugin/available_plugins/ep_disable_change_author_name

and
https://github.com/fourplusone/etherpad-plugins/tree/master/ep_linkify

those plugins do implement very simple features in a few lines

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