Improvements to sketchfab plugin

RESOLVED FIXED

Status

Webmaker
Popcorn Maker
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mjschranz, Assigned: mjschranz)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
1) It's editor is missing a label for the id input.
2) It's id input should be capable of parsing sketchfab URLs e.g: http://sketchfab.com/show/b7LzIm8JrnPw4GBDOMBNGYc39qM
3) We should move it down to the bottom of the plugin list. More of a preference than actual problem.
#2 is a bug in the regex in popcorn.sketchfab.js.

`src = options.src.match( /((show|skfb\.ly)\/)?([A-Za-z0-9]{9,27})/ );`

Notice {9,27} tries to capture the model id, but that "sketchfab" is also 9 characters long :/.
(Assignee)

Comment 2

5 years ago
Ah, well that needs to be fixed and it needs to be obvious that this already was a present feature :P

Comment 3

5 years ago
I agree with :mjschranz. I would put it last in the list order of events.

Moreover, I think we should call the event "3D Model"

Suggested language for UI...

Add a label above the URL field: "Sketchfab.com URL"

Explanatory text below it: "You can include any 3D model from Sketchfab.com in your project. Your browser must support WebGL."
(Assignee)

Updated

5 years ago
Duplicate of this bug: 916318
(Assignee)

Comment 5

5 years ago
As said in the duplicate bug and emails, I've set this plugin to be hidden from our events list until the UI improvements are made.

https://github.com/mozilla/popcorn.webmaker.org/commit/3c5c364dbe8efa157e7b2fe03c5b40e3f131d59e

I haven't pushed this to our staging server, but if production pushes are to be made before this bug is resolved then we will be getting that commit up on prod as well.
Created attachment 806015 [details] [review]
https://github.com/mjschranz/popcorn.webmaker.org/pull/5

- Makes Sketchfab its own editor (doesn't use default).
- Adds blurb & video about Sketchfab to help understand the plugin :).
- Uses some error propagation to tell the user when they've entered a bad url.
Attachment #805305 - Attachment is obsolete: true
Attachment #805305 - Flags: review?(bobby)
Attachment #806015 - Flags: review?(schranz.m)
Comment on attachment 806015 [details] [review]
https://github.com/mjschranz/popcorn.webmaker.org/pull/5

Some easy quick improvements.

Add:


  EditorHelper.selectable( trackEvent, container );

To the editor helper. I noticed there is no select state on the stage container. Usually we get a green border/box shadow when selected. Same with when in edit mode, and hover. Sketchfab only seems to get the hover states. I think your css is overriding the select css? maybe?

Also, why not enable the north west resize handle.

Otherwise r+
Attachment #806015 - Flags: review?(schranz.m) → review-
Actually, now that I'm thinking about borders, I wonder if sketchfab should have one at all?

If I make something with a transparent background, I probably don't want a border either. That way I can overlay it over other images or video in more creative ways.
Created attachment 806065 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/208

Merges with Matt's work, and includes fixes for Scott.
Attachment #806015 - Attachment is obsolete: true
Attachment #806065 - Flags: review?(scott)
Attachment #806065 - Flags: review?(scott) → review+
Matt, can you land this?
Flags: needinfo?(schranz.m)

Comment 12

5 years ago
Commit pushed to master at https://github.com/mozilla/popcorn.webmaker.org

https://github.com/mozilla/popcorn.webmaker.org/commit/dd27d37d0c3eb3b2adc698e6c3f25c9a1b9e0879
Bug 916236 - Sketchfab plugin improvements

[t916236] fix minor usability issues with sketchfab plugin; reenable it's ability to be seen
[t916236] custom sketchfab for a blurb about Sketchfab.com
[t916236] sketchfab background fix & URL error
[t916236] editor helper fixes
(Assignee)

Comment 13

5 years ago
Needs verification.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(schranz.m)
Resolution: --- → FIXED
Attachment mime type: text/plain text/plain text/plain → text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.