-
Notifications
You must be signed in to change notification settings - Fork 60
Support view mode import #577
base: master
Are you sure you want to change the base?
Conversation
@@ -288,6 +288,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`); | |||
if (type === Snippet.ImportType.URL && viewUrlMatch.test(data)) { | |||
/* If importing from a view-mode URL, simply redirect the user */ |
There was a problem hiding this comment.
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?..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -288,6 +288,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`); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
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
@cshashwat , ping on this one, just to get the pull request closed off... |
BTW, we should support importing from any of the Script Lab environment URLs (e.g., localhost or prod should still be able to import a |
No description provided.