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

Markdown editor final chapter: add editor on publisher, comments and conversation of mobile view #7235

Merged
merged 3 commits into from Aug 12, 2017

Conversation

@ghost
Copy link

ghost commented Dec 1, 2016

Publisher:
screen shot 2016-12-01 at 11 44 43

Comments:
screen shot 2016-12-01 at 12 03 47

New conversation:
screen shot 2016-12-01 at 12 07 20

Existing conversation:
screen shot 2016-12-01 at 12 07 12

@ghost ghost changed the title Markdown editor apocalypse : mobile view Markdown editor final chapter: add md editor on publisher, comments and conversation on mobile view Dec 1, 2016
@ghost ghost changed the title Markdown editor final chapter: add md editor on publisher, comments and conversation on mobile view Markdown editor final chapter: add editor on publisher, comments and conversation on mobile view Dec 1, 2016
@ghost ghost changed the title Markdown editor final chapter: add editor on publisher, comments and conversation on mobile view Markdown editor final chapter: add editor on publisher, comments and conversation of mobile view Dec 2, 2016
@@ -11,6 +11,8 @@
initialize: function() {
var self = this;

new Diaspora.MarkdownEditor(".comment_box");

This comment has been minimized.

Copy link
@cmrd-senya

cmrd-senya Feb 18, 2017

Member

Maybe it is out of scope for this PR, but are we okay with the object design where constructor actually performs some job besides the object creation? This decision was made before this PR, and if it was fine back then, then these calls are fine either.

Perhaps it would be a little cleaner to have something like

new Diaspora.MarkdownEditor().renderOn(".comment_box");

Perhaps it is a kind of nit picking, but I felt I have to notice that.

This comment has been minimized.

Copy link
@ghost

ghost Feb 18, 2017

Author

I followed the idioms of Backbone where the HTML element is passed to the constructor.

This comment has been minimized.

Copy link
@cmrd-senya

cmrd-senya Feb 18, 2017

Member

Then it can be done this way:

new Diaspora.MarkdownEditor(".comment_box").render();

My main point is to split object creation and its actual usage.

I don't think it major (and moreover out of the PR scope), but what do others think?

This comment has been minimized.

Copy link
@ghost

ghost Feb 18, 2017

Author

I'm not sure this is big deal. Furthermore, we aren't constructing the object without rendering it. So that's a bit redundant.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 18, 2017

Member

@cmrd-senya What you write seems legit to me, but I'd also like to hear @denschub s opinion on this topic. But please let's do this somewhere else. This is out of scope of this PR. Just open a new code refactor issue or so.

This comment has been minimized.

Copy link
@denschub

denschub Feb 18, 2017

Member

Ideally, you'd have the constructor doing nothing besides internal state. There are multiple reasons for that, but mainly there are very valid reasons to have the render function called again under some circumstances, or to build objects early when the DOM is not even finishing loading, dispatching rendering to a later point in time to increase the apparent performance.

I agree that this is something we could change, but as @svbergerem said, let's not do that here.

Copy link
Member

Flaburgan left a comment

Alright, this looks good! The code is pretty simple, I didn't check if there is a simpler way to write the CSS but at least on Firefox everything is displayed fine.
I will probably merge that on d-fr to test it on production more deeply.
A small remarks, I would have preferred to see the list icons displayed instead of the code one as I feel code is mostly for tech people (who probably directly type markdown) but this should be changed in an other PR as this one is pretty clean and easy to review.

Thanks for your work!

@cmrd-senya

This comment has been minimized.

Copy link
Member

cmrd-senya commented May 11, 2017

I did some tests of the markdown editor and found some issues with it.

  1. In the desktop publisher when the publisher is full with lines and I press H the publisher text area stretches to fit the text. In the mobile publisher when it is full and I press H it doesn't stretch, but the text is inserted. See video: https://b2aeaa58a57a200320db-8b65b95250e902c437b256b5abf3eac7.ssl.cf5.rackcdn.com/media_entries/13845/out-3.ogv

  2. Also, it is not a bug of this PR, but when I consequently push H button it adds and removes ### on the same line (toggles the heading) but it always adds a newline, so on removing of ### it doesn't delete the newline, so pressing the H button continuosly with the same line makes the text area filled with newlines. It happens both on mobile and desktop.

  3. Another thing which is not directly related to this PR, but related to publisher - is that for some reason I can't insert any other link type besides http:// and https://. User may want to instert any link type, including ftp://, bitcoin:, xmpp:, mailto:, magnet:, and many others. I see no reason to limit input link types for user. But it is different for images, since we won't be able to embed any images besides ones addressed via http and https.

I think at least issue number 1 should be addressed before we can merge this.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented May 19, 2017

Points 2 and 3 don't belong to this PR.

What you're saying in point 1 is that the publisher doesn't expand correctly when we press the "insert title" button. I'll have a look but I don't have an idea what's causing that on the top of my head right now.

@cmrd-senya

This comment has been minimized.

Copy link
Member

cmrd-senya commented May 19, 2017

Points 2 and 3 don't belong to this PR.

That's true, but I think it would be nice to fix them before we actually make a release of this feature.

What you're saying in point 1 is that the publisher doesn't expand correctly when we press the "insert title" button. I'll have a look but I don't have an idea what's causing that on the top of my head right now.

Well, that doesn't happen in desktop, it expands there correctly and also expansion works in mobile in other cases, so must be something wrong with the styles I guess. Do you feel okay for an investigation?

@cmrd-senya

This comment has been minimized.

Copy link
Member

cmrd-senya commented Jul 15, 2017

Okay, I checked points 2 and 3, these are upstream issues of bootstrap-markdown. I created issues toopay/bootstrap-markdown#292 and toopay/bootstrap-markdown#293.

Copy link
Member

svbergerem left a comment

I'll add the requested changes in a commit myself.

@@ -27,6 +27,7 @@

.btn-group {
margin-bottom: 8px;
&:first-child { margin-left: 0; }

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

It looks to me like there is no margin set anywhere else, so this line looks unnecessary to me.

This comment has been minimized.

Copy link
@SuperTux88

SuperTux88 Aug 12, 2017

Member

This line was to move the first group to the beginning (same margin as from the top/bottom):

Without this line:

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2017

Member

Thanks!

clear: both;
content: ' ';
display: table;
}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

I removed these lines and I can't spot any difference for comments, conversations and new status messages.

.md-editor {
border: 1px solid $light-grey;
border-radius: $btn-border-radius-base;
overflow: hidden;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

overflow: hidden is already added to the textarea by autosize and this line here looks unnecessary.


textarea {
border: 0;
border-radius: 0;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

The desktop markdown editor CSS already removes the border, so one can remove these two lines.

textarea {
border: 0;
border-radius: 0;
min-height: 6rem;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

The size is set by autosize and this min-height doesn't seem to make any difference.

border: 0;
border-radius: 0;
min-height: 6rem;
resize: none;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

Already set by autosize.

border-radius: 0;
min-height: 6rem;
resize: none;
width: 100%;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

Already set in comments, conversations and new status messages by .form-control.

@@ -42,8 +44,6 @@ h3 { margin-top: 0; }
.clear { clear: both; }
#main { padding: 56px 10px 0 10px; }

textarea { resize: vertical; }

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

The same code is a few lines above.

textarea {
min-width: 100%;
max-width: 100%;
}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

width: 100% already set for .form-control.

min-width: 100%;
max-width: 100%;
}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

The element doesn't exist anymore.

background: transparent;
}
}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2017

Member

The #publisher_textarea_wrapper only contains the photodropzone.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 11, 2017

Would be great if someone else could review my changes.

@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Aug 12, 2017

Would be great if someone else could review my changes.

Done, see #7235 (comment)

Otherwise looks good to me 👍

svbergerem added 2 commits Aug 11, 2017
@svbergerem svbergerem merged commit f4cdec0 into diaspora:develop Aug 12, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage remained the same at 85.826%
Details
svbergerem added a commit that referenced this pull request Aug 12, 2017
Markdown editor final chapter: add editor on publisher, comments and conversation of mobile view
@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 12, 2017

Thanks!

@svbergerem svbergerem removed this from Needs review in Pull Requests Aug 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.