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

Adding bootstrap-markdown editor #6551

Merged
merged 1 commit into from Aug 12, 2016
Merged

Conversation

@ghost
Copy link

ghost commented Nov 17, 2015

Adding Bootstrap Markdown editor to publisher.

  • preview rendering with markdown-it
  • render location
  • render poll
  • render photos
  • jasmine test
  • fix cukes
  • localization
  • UT for MarkdownEditor
options.onPreview = previewCallback;
}
else{
options.disabledButtons = ["cmdPreview"]

This comment has been minimized.

Copy link
@diaspora-code-review
@@ -6,3 +6,4 @@
// Plugins

@import "bootstrap3-switch";
@import "bootstrap-markdown/bootstrap-markdown.min";

This comment has been minimized.

Copy link
@diaspora-code-review

diaspora-code-review Nov 17, 2015

Prefer single quoted strings

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 17, 2015

@jhass: I'm having a problem with the asset pipeline on this:

screen shot 2015-11-17 at 22 35 50

Do you know where it comes from?

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Nov 18, 2015

This would be pretty awesome if you can pull it off! The markdown variant seems like the correct one, though I the preview should probably be disabled since it probably wont be able to render all the codes diaspora supports? And we already have a preview.

This would especially help new users to settle in who have never heard about markdown.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

I the preview should probably be disabled since it probably wont be able to render all the codes diaspora supports?

I plan to use our rendering engine. One can sepcify a rendering function.

This would especially help new users to settle in who have never heard about markdown.

Yep, and it would clearly easier markdown formatting on mobile.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

I think my problem could be related to this. Since there seems to be no way to prevent bundler to perform dependency resolution, I'll try the solution in the last answer tonight.
Edit: Ok, nice, the fix works!

markdowneditor

@@ -2,6 +2,10 @@
# licensed under the Affero General Public License version 3 or later. See
# the COPYRIGHT file.

Rails.configuration.assets.paths.reject! do |path|
path.include?('rails-assets-bootstrap') && !path.include?('rails-assets-bootstrap-markdown')

This comment has been minimized.

Copy link
@diaspora-code-review

diaspora-code-review Nov 18, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Nov 18, 2015

So this looks like an attempt to solve #5801. Is this conform to what we said there, == a publisher close to what github does (a write tab, not wisiwig, and a preview tab)?

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

Yup! :)

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Nov 18, 2015

Nothing against a preview tab, but these buttons always look so clunky and messy :/

@@ -2,6 +2,10 @@
# licensed under the Affero General Public License version 3 or later. See
# the COPYRIGHT file.

Rails.configuration.assets.paths.reject! do |path|

This comment has been minimized.

Copy link
@jhass

jhass Nov 18, 2015

Member

Please add a comment explaining this hack.

This comment has been minimized.

Copy link
@ghost

ghost Nov 18, 2015

Author

Sure. Ninja commit from work ;)

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Nov 18, 2015

Also make sure to render the preview with our existing markdown-it pipeline, if it uses a different renderer it's useless.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

Nothing to worry, preview will be rendered with our engine.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Nov 18, 2015

Will be great if you can get this working! Just a thought, to save visual clutter and space, could you have the format icons spaced along a single bar rather than each being in a button? Something like this:

_format

It might even be best to merge this bar with the notification about using Markdown, to save space.

I'd also suggest streamlining the new preview function with the existing one. There was discussion before about changing the preview button to a tab, which seems a good idea to me - but whether or not you decide to tackle that in this PR, I don't think there's a need for a second preview button.

Thanks for your work!

@DeadSuperHero

This comment has been minimized.

Copy link
Member

DeadSuperHero commented Nov 18, 2015

That looks great, goob! I agree that a simple formatting bar might do the
trick here.

On Wed, Nov 18, 2015 at 1:03 PM, goob notifications@github.com wrote:

Will be great if you can get this working! Just a thought, to save visual
clutter and space, could you have the format icons spaced along a single
bar rather than each being in a button? Something like this:

[image: _format]
https://cloud.githubusercontent.com/assets/2530168/11250958/e0fb08a2-8e26-11e5-8a83-b706ec7e8e8f.png

It might even be best to merge this bar with the notification about using
Markdown, to save space.

I'd also suggest streamlining the new preview function with the existing
one. There was discussion before about changing the preview button to a
tab, which seems a good idea to me - but whether or not you decide to
tackle that in this PR, I don't think there's a need for a second preview
button.

Thanks for your work!


Reply to this email directly or view it on GitHub
#6551 (comment).

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

It might even be best to merge this bar with the notification about using Markdown, to save space.

I thought about doing this. But I fear it could be controversial.

I'd also suggest streamlining the new preview function with the existing one. There was discussion before about changing the preview button to a tab, which seems a good idea to me

Yep, that's planned ;)

I don't think there's a need for a second preview button.

Current existing one will be annihilated! >:D
Second shot:

screen shot 2015-11-18 at 20 24 39
screen shot 2015-11-18 at 20 38 02

#6537 will have to get merged before I can continue working on the preview function.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

Here is a design proposition based on what @goobertron proposed:

screen shot 2015-11-18 at 21 03 45

I don't really like it. I find it messy and I fear about the risks of misclick. Particularly on a mobile phone.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Nov 18, 2015

I would prefer to see the buttons at the bottom to not distract the user, but we then need a way to clearly differentiate what is markdown insert and what is post addition (polls, picture, location).

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

I would prefer to see the buttons at the bottom to not distract the user

I can't choose that, unhopefully. Besides, Having the formatting buttons at the top pretty a standard.

@denschub

This comment has been minimized.

Copy link
Member

denschub commented Nov 18, 2015

I have to agree with Jonne at

Nothing against a preview tab, but these buttons always look so clunky and messy :/

Idea: One could use the "Markdown" link that's already there to trigger the editor to load.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

Idea: One could use the "Markdown" link that's already there to trigger the editor to load.

My opinion is that it has to be easy for inexperimented users first. So buttons have to be displayed by default.

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Nov 18, 2015

My opinion is that it has to be easy for inexperimented users first. So buttons have to be displayed by default.

I agree our default should be "common" users. Or have a setting to switch the default. But default for new users should be to show the buttons.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Nov 18, 2015

Let's first see if we can make the buttons as subtle and integrated as possible. If that's still too much for used users, then we'll see what we can do :)

@denschub

This comment has been minimized.

Copy link
Member

denschub commented Nov 18, 2015

"Common users", at least on Geraspora, do not use any formatting but only publish plain text posts. ;)

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Nov 18, 2015

"Common users", at least on Geraspora, do not use any formatting but only publish plain text posts. ;)

Maybe they don't know how to format posts.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Nov 18, 2015

Here is how I see the publisher (note that the images have a width of 650px so the mockups work on all screens):

First proposition:

Writing
publisher-write
Reading
publisher-read

Second proposition:

Writing
publisher-write2
Reading
publisher-read2

About the second proposition:
To move the poll, upload and location icon on the top right allow to:

  • Clearly separate them from the "insert markdown" button, so no confusion for the user (we could even add text like "Insert markdown" and "attach content" to legend what the buttons are doing)
  • Allow to completely hide the bottom bar when previewing, we have imo a better idea about how the post will look like.
    A down side is that it puts them away from the "publish" button, meaning the user is not encourage / remembered to use them.
@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

Nope, @Flaburgan, your proposition won't be possible. I can't just place the buttons where I want because they are created by the plugin. So, the way it goes, I'd have to... rewrite the plugin. Plus, I explained that small buttons like on your mocks causes huge risks on misclick.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Nov 18, 2015

@AugierLe42e my mockups describe how the publisher can look like, they don't include any technical constraint. And I think we should first imagine and design what would be the best, and then see what are the technical limitations.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Nov 18, 2015

@AugierLe42e my mockups describe how the publisher can look like, they don't include any technical constraint.

So, maybe that's not the right place ;)

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Nov 18, 2015

Yeah I should have created them when I opened #5801 but now it looks like this PR is an attempt to close this issue (thanks for that!) so I post them here.

We already have a preview button and we use our own render engine. So to not use the plugin to do that seems fine for me. I don't think my proposed design adds a lot more work ;)

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 11, 2016

After reloading the stream page when there is still some content in the publisher:

publisher after reload

@@ -63,10 +63,19 @@
#main_stream .stream_element {
padding: 10px;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 11, 2016

Member

You also added padding: 10px; to the .stream_element rule so I guess you can remove this here.

#poll_creator_container
-# handlebars template
.poll-creator-container#poll_creator_container
-# handlebars template

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

I guess you can move this comment back in there.

list: "list text here"
quote: "quotation text here"
code: "code here"

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

trailing whitespaces

end

Then /^I should see "([^"]*)" in the preview$/ do |text|
with_scope(".publisher-textarea-wrapper .md-preview") do
current_scope.should have_content(text)

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

expect(current_scope).to have_content(text)


Then /^I should not see "([^"]*)" in the preview$/ do |text|
with_scope(".publisher-textarea-wrapper .md-preview") do
current_scope.should_not have_content(text)

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

expect(current_scope).to_not have_content(text)


Then /^I should not be in preview mode$/ do
with_scope(".publisher-textarea-wrapper") do
current_scope.should_not have_css(".md-preview")

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

expect(current_scope).to_not have_content(".md-preview")

Then the preview should not be collapsed

And I preview the post
# Then the previewed should be collapsed

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

I think it would be great to keep the test.
Then the preview should not be collapsed

with_scope("publisher-textarea-wrapper .md-preview") do
expect(find(".stream_element .collapsible", match: :first)).to have_css(".expander")
end
end

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

You don't use this. (Drop it)

end

Then /^the preview should not be collapsed$/ do
find(".post_preview").should_not have_selector('.collapsed')
find(".post_preview").should have_selector('.opened')

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

Keep it and just modify the test so it works with the new preview.

"author": app.currentUser ? app.currentUser.attributes : {},
"mentioned_people": mentionedPeople,
"photos": photos,
"frame_name": "status",

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

I'm unable to find this anywhere else so maybe we can just drop this?

"author": {},
"mentioned_people": [],
"photos": [],
"frame_name": "status",

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

drop this here, too

"location": ["", ""],
"interactions": {"likes": [], "reshares": [], "comments_count": 0, "likes_count": 0, "reshares_count": 0},
"poll": undefined
});

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

I heard factories are a thing now. ;) postWithPoll might fit.

This comment has been minimized.

Copy link
@ghost

ghost Aug 12, 2016

Author

Wow! Nice! How do you call it? Factories? :D

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

https://github.com/diaspora/diaspora/blob/develop/spec/javascripts/jasmine_helpers/factory.js#L184

spec/javascripts/app/views/poll_view_spec.js:4: this.view = new app.views.Poll({ model: factory.postWithPoll()});

This comment has been minimized.

Copy link
@ghost

ghost Aug 12, 2016

Author

I know. It was a joke but it seems I failed :(

@@ -100,10 +112,10 @@ describe("app.views.Publisher", function() {
});

it("calls removePostPreview", function(){

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

"calls hidePreview"

this.$el = $("#fake-textarea");
Diaspora.I18n.language = "en";
var locale = {
// Localisation

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 12, 2016

Member

localization

}
/* eslint-enable camelcase */
};
Diaspora.I18n.load(locale, "en", locale);

This comment has been minimized.

Copy link
@svbergerem
@ghost

This comment has been minimized.

Copy link
Author

ghost commented Aug 12, 2016

After reloading the stream page when there is still some content in the publisher:

I can't reproduce :/

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 12, 2016

I'm able to reproduce it with Firefox 48 and Firefox Developer (50.0a2 (2016-08-05)):

  1. Go to the main stream.
  2. Enter some text into the publisher.
  3. Reload the page. (Hit F5)
  4. Confirm the alert.
@ghost

This comment has been minimized.

Copy link
Author

ghost commented Aug 12, 2016

indeed

@svbergerem svbergerem merged commit 5c2e241 into diaspora:develop Aug 12, 2016
5 of 6 checks passed
5 of 6 checks passed
pronto/scss Found 2 warnings.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 82.574%
Details
pronto/es_lint Coast is clear!
pronto/haml Coast is clear!
pronto/rubocop Coast is clear!
svbergerem pushed a commit that referenced this pull request Aug 12, 2016
Steffen van Bergerem
Adding bootstrap-markdown editor
@ghost ghost deleted the bootstrap-markdown branch Aug 12, 2016
@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 12, 2016

Thank you! 👍

@svbergerem svbergerem added this to the 0.6.0.0 milestone Aug 12, 2016
@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 13, 2016

Wow, thank you @AugierLe42e! A major UI improvement.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Aug 13, 2016

Thank you @goobertron 😄

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