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

Switch editor from mdedit to simplemde #22

Merged
merged 4 commits into from
Apr 10, 2017
Merged

Switch editor from mdedit to simplemde #22

merged 4 commits into from
Apr 10, 2017

Conversation

Henni
Copy link
Member

@Henni Henni commented Dec 1, 2016

fixes #7

@Henni Henni added the enhancement New feature or request label Dec 1, 2016
@korelstar
Copy link
Member

Is this work in progress or ready to review?

@phsc84
Copy link

phsc84 commented Dec 1, 2016

I just gave it a try (took the simplemde branch and installed it on my Nextcloud 10.0.1). It works so far, but honestly I would prefer a simple toolbar with a edit/preview toggle as the Nextnotes app already provides it. In my opinion, the current imlementation is a bit too minimalistic.

@Henni
Copy link
Member Author

Henni commented Dec 1, 2016

@korelstar feel free to review it. It should already be fully functional.

@PhSc currently I don't see the use of a preview functionality, so I won't add it in this PR. But of course you can open an issue where we discuss this and then it might be added in the future.

@phsc84 phsc84 mentioned this pull request Dec 1, 2016
@korelstar
Copy link
Member

Looks good and the MD interpretation seems to be better working than before. But

  • Line-spacing is smaller than before. It must not be as high as before (line-height: 1.6em), but a little bit more would be nice.
  • New max-width (see add max-width for better readability #16) is lost and is now again 100%.
  • In Firefox (and not in Chromium), there is an ugly horizontal bar between text and word-count (see screenshot below). This could be fixed by setting css .CodeMirror-scroll { overflow-x: auto !important; }
  • It would be nice if links in the text are clickable, but this wasn't the case before anyway, and it seems to be not supported by SimpleMDE.
  • Gulp gives this warning: app/directives/editor.js line 16 col 17 'editorElement' is defined but never used.

simplemde

@Henni
Copy link
Member Author

Henni commented Dec 6, 2016

@korelstar awesome review, thanks! I'll look into these issues.

@Henni
Copy link
Member Author

Henni commented Dec 9, 2016

@korelstar fixed your issues, reimplemented clickable links (yes this feature existed before 😉 ) and rebased.

Regarding clickable links: I implemented it so that it doesn't disturb while you want to write, by requiring to press Ctrl and click to open the link. It might make sense to make this a bit more obvious or change this in the future, because SimpleMDE supports multiple cursors which are placed with Ctrl+Click.
Besides that is the current implementation "glitchy" on Safari and Firefox as they don't support pointer-events.


Remaining todo:

  • write tests

@korelstar
Copy link
Member

👍

But it's a pitty, that URLs don't work on Firefox.

@Henni
Copy link
Member Author

Henni commented Dec 9, 2016

@korelstar they should work sometimes
The problem is that sometimes the cursor is in front of the link so that it can't be clicked.

@korelstar
Copy link
Member

On my installation, it did never work until now 😞

@korelstar
Copy link
Member

korelstar commented Dec 12, 2016

Just noticed, that nested quotes (> > quote) are displayed like first-level quotes (> quote). But the old library did it the same way.

One obvious thing, we did not mention yet, is that all markdown characters (like # or **) were shown in gray (old library) and now in black (simplemde). Gray was very helpful, because it's a good compromise of replacing these special tags by its formatting but still allowing to edit the tag. Now, e.g. for headings, we have these large # signs which are very dominant and make the editing rather technical instead of allowing to concentrate on the content. Maybe it is possible to format them in gray again?

@Henni
Copy link
Member Author

Henni commented Dec 12, 2016

@korelstar good point. Completely overlooked that.
todo:

  • grey out formatting characters

@phsc84
Copy link

phsc84 commented Dec 12, 2016

Once again, I vote for having a small toolbar with the most important formatting options (bold/italic/lists/headings) and an edit/preview toggle. SimpleMDE brings all this with it.

To be honest, we discuss, that we cannot use the file extension '.md' by default, because it is too nerdy and technical (see #24), but we also display all Markdown formatting codes by default. For me this is way more nerdy and technical.

Users are used to have buttons to format text (even here on Github we have a toolbar, and the average Github user is far more nerdy than the average Nextcloud user, I think).

@korelstar
Copy link
Member

@PhSc Personally, I'm fine with a small toolbar with formatting options. I understand, that this would be helpful for people, who are not that familiar with markdown. And people who want a distraction-free GUI can use the new distraction-free mode.

However, I can't see a connection to the default file extension. Non-nerdy people are likely to not use markdown at all, since nobody is forced to use it. These people will never see the nerdy and technical formatting codes. Whereas, forcing the use of a broadly unknown extension could confuse many people.

But again, I'm fine with a small toolbar. But it seems, that the designers don't like it.

@jancborchardt
Copy link
Member

If we do offer formatting then not via a permanent distracting bar, but via a popover when marking text. Check out how Medium does it – just open any article and mark text. It’s just there when you actually need it.

@phsc84
Copy link

phsc84 commented Dec 16, 2016

Fine with me.
Open questions in my opinion:

  • How do we handle the Markdown codes (#, *, etc.)? Should we hide or display them? Displaying them is also a bit distracting IMHO.
  • Do we support both ways of formatting the text?
    • Via popover
    • Via Markdown code

@ThomasDaheim
Copy link

I would really like to see that going into the master! At least one step closer to a WYSIWIG editor...

@spoorun
Copy link

spoorun commented Apr 6, 2017

@ThomasDaheim Developers at simplemde have made it clear they do not intend to develop the ability to edit within preview mode, so it will not meet that requirement alas.
[We should fork OwnNote to a new app that has the ability to store in Markdown (for ease of storage and portability) but enter using either Wywiwyg or Markdown. Woofmark may be usable as an editor(?)]

@ThomasDaheim
Copy link

@olantrust I think we should move such a discussion to another place - not using an old issue that no one looks into anymore :-)

My private roadmap is:

  1. Enable ownNoteEditor to use tinymce/woofmark/whatever the decision is going to be
  2. Push the notes app to switch to woofmark as well :-)
  3. Add to notes app to support all file types that woofmark could handle, so any user can choose between html-like or markup
  4. Fallback: fork ownNote to do updates there <- would only be my fallback, since it would mean building a new android app as well :-(

First point is well under way, the second one doesn't depend on me...

@Henni
Copy link
Member Author

Henni commented Apr 10, 2017

Fails thanks to dependency on old owncloud version...
=> Next todo: use nextcloud for testing

@Henni Henni merged commit e78077b into master Apr 10, 2017
@Henni Henni deleted the simplemde branch April 10, 2017 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace mdEdit
6 participants