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

Import note from url with markdown #1981

Closed
wants to merge 3 commits into from

Conversation

StormBurpee
Copy link
Contributor

Hi guys,

Thought I'd try and create the feature asked for at #923
The solution I came up with is quite friendly, and provides an easy way to put in a URL and convert it to Markdown. Let me know what you guys think!

The new note screen, note the import from url at the bottom
screen shot 2018-05-26 at 4 59 01 pm

The new modal, for adding a url and importing
screen shot 2018-05-26 at 4 59 21 pm

The result in split markdown view
screen shot 2018-05-26 at 4 59 34 pm

The final result of the imported note
screen shot 2018-05-26 at 4 59 51 pm

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

please confirm my review!

and please fix from eslint.
please user yarn instead of npm.

}
}

confirm () {
Copy link
Member

Choose a reason for hiding this comment

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

please add a test!

@kazup01 kazup01 added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels May 28, 2018
@StormBurpee
Copy link
Contributor Author

Moved the function to the dataApi folder, so that a test can be created easier. But I'm struggling to write the test at the moment, so I'll take a look a little later. If someone want's to take a look, feel free too.

import { hashHistory } from 'react-router'
import ee from 'browser/main/lib/eventEmitter'

function validateUrl(str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method could be simplyfied to

return str.matches(new RegExp('^(?:.......'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just used the jQuery implementation for validating URLs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my idea would have introduced the opportunity for null pointers.. So it's good that you ignored my concern :D

@sosukesuzuki
Copy link
Member

@StormBurpee
please follow eslint(tests/dataApi/createNoteFromUrl-test.js).
please fix conflict.

(I'll review the test written by you, later.)

Copy link
Member

@Rokt33r Rokt33r left a comment

Choose a reason for hiding this comment

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

Please remove package-lock.json and use yarn instead of npm when developing.

@Rokt33r
Copy link
Member

Rokt33r commented Jun 8, 2018

Anyway, it looks great first step of the feature. Nice work!

@sosukesuzuki
Copy link
Member

@StormBurpee please confirm our reviews!

@Rokt33r
Copy link
Member

Rokt33r commented Jul 19, 2018

I'm going to close this issue because of the inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants