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

Issue #867 - UI for including screenshot in bug report #893

Merged
merged 7 commits into from
Jan 5, 2016

Conversation

magsout
Copy link
Member

@magsout magsout commented Dec 30, 2015

Ok, so @miketaylr

capture d ecran 2015-12-30 a 21 26 31

<div class="wc-Form-upload-icon" aria-hidden="true">
<span class="wc-Icon wc-Icon--cloud-upload"></span>
</div>
{{ form.image.label(class_='wc-Form-label wc-Form-label--upload') }}

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@miketaylr
Copy link
Member

Whoa, speedy. ⚡ Will play with this in a bit.

@miketaylr
Copy link
Member

This looks awesome @magsout. I'm gonna add an image preview via FileReader (if the browser supports it), otherwise it's hard to know if you've selected something. Once that's in place, I think we can land this and I'll extend it to work with the add-on screenshot stuff (and add drag & drop, etc.)

@miketaylr
Copy link
Member

(that push was just to rebase against master)

@miketaylr
Copy link
Member

We can improve the UX in future issues, a couple ideas:

  1. ability to clear out file selection, right now you have to just select a new image
  2. remove/modify "Add screenshot image" + icon
  3. other stuff...

@miketaylr
Copy link
Member

(it can look bad depending on the image, but that's OK I think for now... incremental improvements, etc.):

@magsout
Copy link
Member Author

magsout commented Dec 31, 2015

@miketaylr we can improve UX before merge if you want. Not a big deal.

@miketaylr
Copy link
Member

@magsout if you have any suggestions, feel free to hack on them. I don't really have any amazing ideas, hence my "let's do it later". :p

When you get a chance, can you review the JS part?

@miketaylr
Copy link
Member

I think I have one idea, let me try to come up with a quick mockup.

@miketaylr
Copy link
Member

Here's one idea, stealing from the original design:

Once we have the upload image read and the background-image set, we hide the label w/ icon and show a yellow bar on the bottom with a link "Click here to remove image" (I guess it would technically be a button, but I think styling as a link would be good). Once the user clicks that, we remove the bg-image, clear out the file input, and show the original "Upload screenshot image">

We could re-use that same bottom bar for the add-on use case where a user needs to check the checkbox (something like "[ ] Select to include screenshot image")

screen shot 2015-12-31 at 3 33 53 pm

What do you think?

@miketaylr
Copy link
Member

r? @magsout what do you think?

@magsout
Copy link
Member Author

magsout commented Jan 5, 2016

wooot @miketaylr awesome 👍

@magsout
Copy link
Member Author

magsout commented Jan 5, 2016

I added a small animation and replaced <div> by <button>.

I have one issue when I removed image uploaded:

capture d ecran 2016-01-05 a 12 35 06

@miketaylr
Copy link
Member

Yay, thanks for cleaning up my markup/CSS ^_^. Let me try to reproduce that error, it's kinda wierd.

@miketaylr
Copy link
Member

Ah, the weird view function error is because it's a button -- it's trying to submit or navigate or something. Will sprinkle some JS to fix it, then merge.

@miketaylr
Copy link
Member

Will sprinkle some JS to fix it

Or just use regular HTML type=button.

@magsout
Copy link
Member Author

magsout commented Jan 5, 2016

Ah yes, I writed role instead of type. Good catch @miketaylr

miketaylr pushed a commit that referenced this pull request Jan 5, 2016
Issue #867 - UI for including screenshot in bug report
@miketaylr miketaylr merged commit ddd48e9 into master Jan 5, 2016
@miketaylr miketaylr deleted the screenshot-bug-report branch January 5, 2016 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants