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

Paste/input gist URL for mobile. Fixes #1283 #1341

Merged
merged 3 commits into from
Jan 4, 2017
Merged

Paste/input gist URL for mobile. Fixes #1283 #1341

merged 3 commits into from
Jan 4, 2017

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Dec 28, 2016

R: all

Part of #1283. On mobile, you'll get a url input:

screen shot 2016-12-28 at 10 28 02 am

On mobile/desktop, you'll be able to paste the URL of a gist. Invalid urls will cause a butter bar warning.

@@ -284,6 +301,44 @@ class LighthouseViewerReport {
}

/**
* Handles changes to the gist url input.
*/
onInputChange(e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this look like on mobile then after entering the url, keyboard prompts done? should we add our own button?

Copy link
Contributor Author

@ebidel ebidel Dec 29, 2016

Choose a reason for hiding this comment

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

The user needs to click outside the input or hit enter on the keyboard for it to fetch the gist. Once that's done (change event fires and the gist is fetched), we go straight to showing the gist.

Copy link
Member

Choose a reason for hiding this comment

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

I doubt anyone will hand-type the URL, but those that do will have to do as eric said.

for those on mobile devices that just paste the URL into the field, it'll trigger immediately after paste. (i just tested it out)

@ebidel ebidel added the viewer label Dec 30, 2016
@ebidel
Copy link
Contributor Author

ebidel commented Jan 3, 2017

@patrickhulce wdyt?

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

generally works for me.

return;
}

const id = url.href.substring(url.href.lastIndexOf('/') + 1);
Copy link
Member

Choose a reason for hiding this comment

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

want to pull it out with a regex instead? seems slightly more robust.

i think the following would work..:

new URL(location.href).pathname.match(/[a-f0-9]{5,}/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ebidel
Copy link
Contributor Author

ebidel commented Jan 4, 2017

PTAL

@paulirish
Copy link
Member

paulirish commented Jan 4, 2017

So the viewer now has a few options:

  1. Paste gist URL from clipboard
  2. Paste report json from clipboard
  3. Drag and drop a report.json file
  4. Click, file chooser, select a report.json file

Is that correct?

If so, I think we could adjust some copy.. Here's a proposal:

  • File input placeholder: "Enter full gist url"
    • "Paste Gist URL or report JSON here"

  • Notification on copy: "Report copied to clipboard"
    • "Report JSON copied to clipboard"

  • Welcome blurb: "Instructions: Click here, drag and drop a json report,
    or paste the URL of a saved gist."
    • "To view report: Paste report JSON or Gist URL. You can also drag 'n drop a report.json file or click to select it."

@ebidel
Copy link
Contributor Author

ebidel commented Jan 4, 2017

Updated the copy. I went wit this:

To view report: Paste a report.json or Gist URL.<br>
You can also drag 'n drop the file or click here to select it.

I kept the input placeholder as "Enter or paste a Gist URL" because "Paste Gist URL or report JSON here" makes it sound like you can paste a json file into the input.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

sgtm!

@paulirish paulirish merged commit 5b4bd1c into master Jan 4, 2017
@paulirish paulirish deleted the mobileui branch January 4, 2017 06:51
@ebidel ebidel mentioned this pull request Jan 4, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants