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

Add markdown toolbar #2517

Closed
wants to merge 19 commits into from
Closed

Add markdown toolbar #2517

wants to merge 19 commits into from

Conversation

onukura
Copy link
Member

@onukura onukura commented Aug 19, 2020

Before submitting a pull-request to GitBucket I have first:

  • read the contribution guidelines
  • rebased my branch over master
  • verified that project is compiling
  • verified that tests are passing
  • squashed my commits as appropriate (keep several commits if it is relevant to understand the PR)
  • marked as closed using commit message all issue ID that this PR should correct

About this PR

This PR add markdown toolbar to resolve #1793
I didn't use any markdown editor such as simpleMDE because of integration with GitBucket dropzone and parsing for preview by renderer.

pr_screenshot (0)

@SIkebe SIkebe added the feature label Aug 19, 2020
Copy link
Member

@SIkebe SIkebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it needs some design discussion (e.g. where to insert new characters, where to focus after the event occurred ...), but overall it looks nice!

I didn't any markdown editor library because of integration with GitBucket dropzone and parsing for preview by renderer.

You wrote these new functions without transpile or polyfill, which other libraries usually do, so they do not work in some browsers (e.g. IE11, Firefox for Android). However I think you don't have to care about such cases as most users uses modern browsers on their PC and you can write markdown by hand in the same manner as before.
Even Microsoft will stop supporting IE on their web services 😄
https://techcommunity.microsoft.com/t5/microsoft-365-blog/microsoft-365-apps-say-farewell-to-internet-explorer-11-and/ba-p/1591666

src/main/webapp/assets/common/js/gitbucket.js Outdated Show resolved Hide resolved
src/main/webapp/assets/common/js/gitbucket.js Outdated Show resolved Hide resolved
src/main/webapp/assets/common/js/gitbucket.js Outdated Show resolved Hide resolved
src/main/webapp/assets/common/js/gitbucket.js Outdated Show resolved Hide resolved
src/main/webapp/assets/common/js/gitbucket.js Outdated Show resolved Hide resolved
src/main/webapp/assets/common/js/gitbucket.js Outdated Show resolved Hide resolved
src/main/webapp/assets/common/js/gitbucket.js Outdated Show resolved Hide resolved
@onukura
Copy link
Member Author

onukura commented Aug 19, 2020

@SIkebe Thank you for your review! I fixed doc for js functions first as you suggested!

@onukura
Copy link
Member Author

onukura commented Aug 19, 2020

@SIkebe

I think it needs some design discussion (e.g. where to insert new characters, where to focus after the event occurred ...),

Actually I configured how those symbols such as ###, **** will be inserted in text and where to focus after event referring to GitHub. (but not completely...).
But still I agree with you opinion that we need discuss about design, like what button we want. where, in what order, etc.
I would appreciate If you guys would have any suggestion.

@onukura
Copy link
Member Author

onukura commented Aug 19, 2020

For reference, I will put gif of each button's behavior.

Heading ###

pr_screenshot_head

Bold **** and Italic __ and Code ``

pr_screenshot_bold_italic_code

Quote > and lists -, 1. and task list - []

pr_screenshot_quoate_list

Link [](url)

pr_screenshot_link

@takezoe takezoe added this to the 4.35.0 milestone Aug 20, 2020
@SIkebe
Copy link
Member

SIkebe commented Aug 20, 2020

I configured how those symbols such as ###, **** will be inserted in text and where to focus after event referring to GitHub. (but not completely...).

I don't think it's necessary to copy all behaviors.
I just want to make sure, are these intended difference?
b_i

@SIkebe
Copy link
Member

SIkebe commented Aug 20, 2020

If it's an empty textarea, extra lines are inserted.

extra-line

@onukura
Copy link
Member Author

onukura commented Aug 20, 2020

@SIkebe

I configured how those symbols such as ###, **** will be inserted in text and where to focus after event referring to GitHub. (but not completely...).

I don't think it's necessary to copy all behaviors.
I just want to make sure, are these intended difference?
b_i

This one is yes. I just didn't support range selector...

If it's an empty textarea, extra lines are inserted.

extra-line

This one is yes too. I know it works like this. But maybe at least this one should be like GitHub...? This is not useful. I will change this at least.

@onukura
Copy link
Member Author

onukura commented Aug 20, 2020

Fixed behavior for list insert when textarea is empty.

pr_screenshot_list

@aadrian
Copy link
Member

aadrian commented Aug 21, 2020

IMHO it looks very good.

Another possibility is that we declare this a "feature preview": we included it in the distribution, to get more feedback from the end-users, so we might improve it later if required.

@onukura
Copy link
Member Author

onukura commented Aug 21, 2020

@aadrian

Thank you for your feedback!

Another possibility is that we declare this a "feature preview": we included it in the distribution, to get more feedback from the end-users, so we might improve it later if required.

Is that mean we implement notification in html to tell users this is "feature preview" and ask feedback?

@aadrian
Copy link
Member

aadrian commented Aug 21, 2020

@onukura

Another possibility is that we declare this a "feature preview": we included it in the distribution, to get more feedback from the end-users, so we might improve it later if required.

Is that mean we implement notification in html to tell users this is "feature preview" and ask feedback?

I don't think we that that much. Just in the Announcement Text like this https://gitbucket.github.io/gitbucket-news/gitbucket/2020/07/26/gitbucket-4.34.0.html (but for the next version), we make a special Section where we say instead of "New Feature xyz" - "Feature Preview xyz", and ask the users to try it and give us feedback so that we can change it in the final form.

@onukura
Copy link
Member Author

onukura commented Aug 22, 2020

@aadrian That’s sounds good to me!

@onukura
Copy link
Member Author

onukura commented Aug 22, 2020

Behavior updated. I guess now it's GitHub compatible.

pr_screenshot_list

Handle branch contains / properly in branch settings
@onukura onukura requested review from takezoe and SIkebe August 22, 2020 17:34
* @param {Number} posOffset
* @returns {String}
*/
function mdeDecWord(id, pattern, posOffset){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do Dec stand for? Decorate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Decorate. I fixed these unclear function names 933cf5a

Copy link
Member

@takezoe takezoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this with Chrome on MacOS and some behaviors were weird.

  • New lines are inserted after the current line when I decorate a word
  • In some cases, new line is inserted even before the selected line or - is inserted to another line
  • Undo doesn't work after modifying text using toolbar
  • It would be nice if it could make a list from multiple lines

At least, inserting unnecessary new lines and inserting prefixes to wrong location must be fixed.

@onukura
Copy link
Member Author

onukura commented Aug 27, 2020

I will revise this PR as new PR because it got messy...

@onukura onukura closed this Aug 27, 2020
@onukura onukura mentioned this pull request Aug 27, 2020
6 tasks
@onukura onukura removed this from the 4.35.0 milestone Aug 27, 2020
@onukura onukura deleted the feat-md-editor branch August 28, 2020 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Add toolbar and help to Markdown editor
4 participants