Skip to content
This repository was archived by the owner on Nov 16, 2023. It is now read-only.

Support view mode import #577

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support view mode import #577

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd4ccc8
Support view mode import
Aug 18, 2017
32e24f1
Add more robust regex
Aug 18, 2017
e40e84a
Merge branch 'master' into support-view-import
Zlatkovsky Sep 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/client/app/effects/snippet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,13 @@ export class SnippetEffects {
case Snippet.ImportType.SAMPLE:
case Snippet.ImportType.URL:
case Snippet.ImportType.GIST:
let viewUrlMatch = new RegExp(`^${environment.current.config.editorUrl}\/#\/view`);
Copy link
Member

Choose a reason for hiding this comment

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

I worry about taking semi-arbitrary strings and stuffing them into a regez. What if they contain regex-special characters. I would do a string match first instead, then trim out everything post the root URL, and regex only that.

Copy link
Member

Choose a reason for hiding this comment

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

Also, should ignore capitalization (e.g., "VIEW")

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like there is a lodash method to escape a regular expression. We can probably just use this one:
_.escapeRegExp

if (type === Snippet.ImportType.URL && viewUrlMatch.test(data)) {
/* If importing from a view-mode URL, simply redirect the user */
Copy link
Member

Choose a reason for hiding this comment

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

I don't get it. Don't we want to import rather than navigating the user away?..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By "import" do you mean actually importing the snippet and opening it in editor mode? The view mode already does an import action but doesn't save the snippet to local storage. We now want the user to be able to paste in a "view-url", but now they can edit that snippet? I thought that was accomplished by the open in playground feature.

Copy link
Contributor Author

@cshashwat cshashwat Aug 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Right now the view mode has access to Angular that parses the URL and gives it access to the different params, such as host, gist vs sample, id, etc.. If we wanted to do it in the actual import feature, we would need our own regex matching and URL validation, in addition to the view mode code that already does this. Not sure if we want to do that?

Copy link
Member

Choose a reason for hiding this comment

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

The "import" dialog in Script Lab should allow you to either paste in a GIST or a view URL pointing to a GIST -- and we should treat both as equally valid.

We can talk more in person if you want.

window.location.href = data;
}


let id = null;

const match = /https:\/\/gist.github.com\/(?:.*?\/|.*?)([a-z0-9]{32})$/.exec(data);
Expand Down