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

Use custom JSX implementation of GitHub's markdown toolbar that works in IE #8

Merged
merged 4 commits into from
Sep 6, 2019

Conversation

dsevillamartin
Copy link
Member

@dsevillamartin dsevillamartin commented Jul 10, 2019

Fixes flarum/framework#1702.
Fixes flarum/framework#1671.
Fixes flarum/framework#1679.

image

  • Created MarkdownToolbar and MarkdownButton elements
    • Replacement for custom HTML elements
  • Brought the textarea logic from @github/markdown-toolbar-element into flarum/markdown
    • Wasn't sure how we would want the file structure, I separated the functions into 3 different files
    • Modified some code so that it works in IE, two functions needed pollyfill
      • String#startsWith, String#endsWith
  • Buttons that support hotkeys now have the proper meta key in the tooltip
    • macOS:
    • other: ctrl

  • Do we want to have the styles or button configurations stored somewhere else ?
  • We'll probably need to somehow include the LICENSE as a bunch of code was copied and modified from there.
    • Perhaps in those comments that don't disappear when compiling. Something like this ?

      /*!
       * Includes modified code from GitHub Markdown Toolbar Element
       * https://github.com/github/markdown-toolbar-element/
       * 
       * Original Copyright GitHub, Inc.
       * Released under the MIT license
       * https://github.com/github/markdown-toolbar-element/blob/master/LICENSE
       *
       * Date: 2019-07-10T16:37Z
       */

Copy link
Contributor

@franzliedke franzliedke left a comment

Choose a reason for hiding this comment

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

Awesome, nice job!!!

Yes, we definitely need to mention GitHub's license as well. I'd say we definitely need to add the following line to our LICENSE file:

based on code Copyright (c) 2017-2018 GitHub, Inc.

I am unsure about the rules for bundled and minified JS code. Does that need to include license information as well? Especially if the code, like ours, is available open-source...

js/src/forum/index.js Outdated Show resolved Hide resolved
return this.substring(pos, pos + search.length) === search;
}
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these taken from? Any other licenses we need to mention?

Copy link
Member Author

@dsevillamartin dsevillamartin Aug 2, 2019

Choose a reason for hiding this comment

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

These are taken from the MDN polyfils @ https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith#Polyfill & https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/endsWith#Polyfill.

However, it looks like they first is taken from https://github.com/mathiasbynens/String.prototype.startsWith, so perhaps we want to use the NPM packages instead to avoid licensing issues. Upon further look though, the code from it is way more complicated compared to these 8 lines

Looks like simply including /*! https://mths.be/startswith v0.2.0 by @mathias */ above it might be sufficient.

js/src/forum/util/apply.js Outdated Show resolved Hide resolved
@dsevillamartin
Copy link
Member Author

dsevillamartin commented Aug 3, 2019

@franzliedke To include the license in the dist file, one uses /*! as compilers do not remove comments declared with /*!.

I'm not sure if what I wrote would suffice. We could most likely simplify it (and probably should, to reduce bundle size), following one of the examples found @ Open source licensing notices in Web Applications.

Most likely use the following format, though only with the ! once instead of 3 times (once for each file, as I separated the code to make it (hopefully) more readable).

/*!
 * Copyright OWNER NAME. Licensed under NAME LICENSE HERE
 * See license text at http://example.com/license
 */

so use something like

/*!
 * Original Copyright GitHub, Inc. Licensed under the MIT License.
 * See license text at https://github.com/github/markdown-toolbar-element/blob/master/LICENSE.
 */

Example with the String.prototype.startsWith polyfill:
image

@dsevillamartin
Copy link
Member Author

We might want to consider bringing the base implementation into core. That way, other extensions can add buttons for things like BBCode without having flarum/markdown enabled.

Thoughts ?

@luceos
Copy link
Member

luceos commented Sep 6, 2019

@datitisev can we update/create an issue so that we keep track of that?

@luceos luceos merged commit 50cbe1f into master Sep 6, 2019
@luceos luceos deleted the ds/1702-fix-toolbar-in-ie-11 branch September 6, 2019 19:24
@franzliedke
Copy link
Contributor

Why was this merged without a single copyright notice?

@dsevillamartin
Copy link
Member Author

@franzliedke Oh, I thought I had taken care of it, and mentioned so in the meeting.

The following should be fine (add to all 3 files that use Github code, but only one should use /*!, the rest just /*)

/*!
 * Original Copyright GitHub, Inc. Licensed under the MIT License.
 * See license text at https://github.com/github/markdown-toolbar-element/blob/master/LICENSE.
 */

@franzliedke
Copy link
Contributor

Yes, please add that in master.

@dsevillamartin
Copy link
Member Author

I had to add it to index.js because, for some reason, webpack only keeps comments starting with /*! (or maybe any comment) from the root level... ? Any comment that I put in the js/src/forum/utils folder gets deleted... not sure why.

Also added the line to LICENSE.

askvortsov1 pushed a commit that referenced this pull request Mar 11, 2022
Use custom JSX implementation of GitHub's markdown toolbar that works in IE
askvortsov1 pushed a commit that referenced this pull request May 10, 2022
Use custom JSX implementation of GitHub's markdown toolbar that works in IE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants