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

No more CodeMirror #5031

Merged
merged 2 commits into from Mar 17, 2015

Conversation

@ErisDS
Copy link
Member

commented Mar 13, 2015

Now rebased on ember-cli, and with improved image upload handling

closes #4368, fixes #1240 (spellcheck)

  • Drop CodeMirror in favour of a plain text area
  • Use rangyinputs to handle selections cross-browser
  • Create an API for interacting with the textarea
  • Replace marker manager with a much simpler image manager
  • Reimplement shortcuts, including some bug fixes
@ErisDS ErisDS referenced this pull request Mar 13, 2015
3 of 7 tasks complete
@ErisDS

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2015

There was a bug in the original PR such that image uploads were only working if you used the full image tag ![](), whereas the dropzone is triggered from as little as ![].

The if statement here: ErisDS@98356eb#diff-36b6e407d6ee15870d0692e86d7e3dd8R34 handles that case (I should probably add a comment to explain).

One thing I have noticed whilst playing around with this editor, is that the onChange event that is now the trigger for autosave doesn't fire when images get uploaded. It is possible that other non-human input e.g. shortcuts (i.e. where JS does the insert into the textarea) may also be affected.

Still wanted to get this version up to see how it fairs against Travis. Code review & testing would be much appreciated.

@ErisDS ErisDS force-pushed the ErisDS:new-editor branch Mar 13, 2015

@ErisDS

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2015

Comment added to the image handler + I've found the magic bullet for ensuring that autosave gets triggered by uploads, shortcuts and any other programmatic text manipulation in the same way as typing:

ErisDS@4854db1#diff-c54414e2bf948de8c8eabe735323a7d9R97

I think this might be ready 😧

@ErisDS ErisDS force-pushed the ErisDS:new-editor branch Mar 16, 2015

@novaugust

View changes

core/client/app/components/gh-ed-editor-scroller.js Outdated
import Ember from 'ember';
import setScrollClassName from 'ghost/utils/set-scroll-classname';

var EditorScroller = Ember.Component.extend({

This comment has been minimized.

Copy link
@novaugust

novaugust Mar 16, 2015

Member

I'm looking around, and it doesn't look like this gets used anywhere. Dead code?

@novaugust

View changes

core/client/app/mixins/ed-editor-api.js Outdated
return this.$().getSelection();
},

getLineToCursor: function () {

This comment has been minimized.

Copy link
@novaugust

novaugust Mar 16, 2015

Member

Some docs on these api methods would be cool, but not crucial - just took me a moment to think of what "getLineToCursor" does

@novaugust

View changes

core/client/app/mixins/ed-editor-api.js Outdated

var EditorAPI = Ember.Mixin.create({
getValue: function () {
return this.get('element').value;

This comment has been minimized.

Copy link
@novaugust

novaugust Mar 16, 2015

Member

Just code-style-wise, this threw me for a loop for a bit. I think our convention's been to work with this.$() like you did in getSelection, rather than going for the raw dom element

@@ -1,5 +1,4 @@
/* jshint node:true, browser:true */

This comment has been minimized.

Copy link
@novaugust

novaugust Mar 16, 2015

Member

I can't remember why we have two copies of this file now. Did ember-cli's app.import(...) fail with files that were outside the scope of the project? That sounds right..

This comment has been minimized.

Copy link
@ErisDS

ErisDS Mar 16, 2015

Author Member

I wouldn't worry about it - as soon as this one is merged I have an incoming PR to fix this properly.

@novaugust

This comment has been minimized.

Copy link
Member

commented Mar 16, 2015

Ack... I went off and was making comments on a commit and lost track of what I said and can't find them again. #fail .

Anyhoo, rather than doing the onChange action, we can just hook our editor's value into ember's observable network and use the notifyPropertyChange method: http://emberjs.com/api/classes/Ember.Observable.html

@novaugust

This comment has been minimized.

Copy link
Member

commented Mar 16, 2015

Otherwise, everything looks good 'nuff to me ;) Just get rid of the dead code and ping me and i'll hit das button

PaulAdamDavis and others added 2 commits Nov 3, 2014
Textarea Editor CSS Improvements
References #4373

* Consistent line-height & padding cross-browser
* Remove drag handle from textarea
* Partially clean up the CSS to more closely match the new standard
No more CodeMirror
closes #4368, fixes #1240 (spellcheck), fixes #4974 & fixes #4983 (caret positioning bugs)

- Drop CodeMirror in favour of a plain text area
- Use rangyinputs to handle selections cross-browser
- Create an API for interacting with the textarea
- Replace marker manager with a much simpler image manager
- Reimplement shortcuts, including some bug fixes

@ErisDS ErisDS force-pushed the ErisDS:new-editor branch to a7b82f4 Mar 17, 2015

@novaugust

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

woop woop
screen shot 2015-03-17 at 11 18 20

novaugust added a commit that referenced this pull request Mar 17, 2015

@novaugust novaugust merged commit 60dcfff into TryGhost:master Mar 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ErisDS ErisDS deleted the ErisDS:new-editor branch Aug 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.