iphands / Meltdown

Meltdown (Markdown Extra Live Toolbox): A JQuery plugin that adds Markdown Extra live previews, and a toolbar for common markdown actions.
141 stars 24 forks source link

More customization #9

Closed jlgrall closed 9 years ago

jlgrall commented 10 years ago

Hello again,

I would like to change a few things in Meltdown to make it more easily customizable:

About the addToolTip() and the jQuery.qtip: a more simple solution could be to show the message in the preview area when the user clicks on TECH PREVIEW in the preview header.

I will probably add a simple JS API to interact with the meltdown editor. It will probably be something like a jquery-ui widget, but simpler and not requiring jquery-ui. Thus I will need to refactor parts of the code.

jlgrall commented 10 years ago

Is there any reason to the preview being hidden by default ?

I will probably change it and add the option: autoOpenPreview = true by default.

iphands commented 10 years ago

I like the options. Feel free to add them all... but I would rather have the default be preview closed.

jlgrall commented 10 years ago

To me it doesn't make sense to use a markdown editor if we don't want to use the preview. Why not use a text editor in this case ? If it is because the preview takes too much space, I am adding options to choose how big it should be (or even allow the size to adapt to its content).

I made some good progress. You can see here: https://github.com/jlgrall/Meltdown/tree/customize

Main changes:

Notes:

API changes:

iphands commented 10 years ago

Yikes, I really need legacy jQuery support for the time being. Can we write a bit of code (even if it is a hack) as a workaround for 1.7.2?

Same as above, unfortunately IE8 is our lowest supported browser atm.

Sweet!

Works for me.

Also works for me

After this is merged in I will probably refactor to make this an AMD/require.js module. It is the way we are rolling these days.

iphands commented 10 years ago

"""To me it doesn't make sense to use a markdown editor if we don't want to use the preview.""" This is one thing I am going to stand firm on. Until https://github.com/iphands/Meltdown/issues/1 is resolved I am going to keep the box hidden until the user clicks to show a preview.

Even after issue #1 is fixed I still feel that the toolbox alone is an added benefit, and expanding the preview is something I would rather not see as the default option. Having the auto_open option should be all we need.

jlgrall commented 10 years ago

Thanks for the feedback.

I will leave the auto open to false then.

For IE 8, I will see what I can do. Unfortunately, I only have access to an IE 7 on WinXP as dual-boot, and no other IE. I will keep using cross-browser code, and then, when I have time, I will try to test it on IE. But it is at the bottom of my priority list for now...

For jQuery 1.7.2, I think I could simply disable the preview slide animation in fullscreen mode. The progress callback is used to slide the editor in the opposite direction from the preview.

Making it an AMD/require.js module is a good idea :)

Another question, do you mind if I detab the code in the global self-executing function ? I usually do this for all my scripts that are in a self-executing functions. It saves horizontal space and file size.

As a possible workaround for #1, I considered the possibility of using webworkers for the parsing. The interesting point is that webworkers can be interrupted at any time if they timeout. When they time out, we can display a warning and a button "Force parsing" in case the user has a slow computer. Then a button "Interrupt parsing". This warning could be displayed in the preview, just like the "TECH PREVIEW" warning is displayed. Well, I haven't decided yet...

I never saw the infinite loop from #1. Does it happen often ? Do you know a markdown text that triggers it ?

jlgrall commented 10 years ago

Another idea: sync the editor and preview scroll positions.

I think a way to go could be by having the parser add to each generated element, its source line number: https://github.com/tanakahisateru/js-markdown-extra/issues/34

Any better idea ?

jlgrall commented 10 years ago

I made a layout change, I hope you are ok with it, otherwise just tell me...

While doing the sidebyside feature, I saw that the toolbar could become really too small, which resulted in accessibility problems but also in layout bugs. So I decided to put the toolbar at the very top. It stays always over the editor and when the editor and the preview are side by side, the toolbar stays at the same place over both the editor and the preview.

There are still some troubles when the toolbar is not large enough: the wrapped controls are pushing the following content downwards. And I cannot overflow: hidden the toolbar, because of the submenus...

jlgrall commented 10 years ago

Hello again, I am now at the end of what I wanted to add for this PR.

Last changes:

Notes:

To be done:

jlgrall commented 10 years ago

I tested and fixed the IE 8 issues. It now works on IE 8 with jQuery 1.7.2 or newer.

I cannot test with IE 9.

In the future, removing support for IE 8 and jQuery 1.7.2 should be easy because the relevant code is mostly isolated from the main code.

I am going to make rangyinputs a required dependency, because the toolbar is pretty useless without it and it is a small dependency anyway.

I am currently updating the documentation (the README.md file).

jlgrall commented 10 years ago

As I didn't receive any answer for the last 2 weeks, I decided to go on. I hope you agree with what I did.

It is now ready for a pull request. You can add the AMD/require.js module code and add that info in the README.md and the changelog.

I added the git tag v0.1 to the last commit before I started this branch. That was the last version that was compatible with IE 6-7 and which had not too much incompatible differences with your initial commit.

When you merge this branch, you should then add the v0.2 git tag and update the release date in the README.md changelog and in the header of jquery.meltdown.js

Have a good day.

Edit: info for AMD/require.js module:

iphands commented 10 years ago

Cool. Thanks I will have a look and merge sometime soon.