875580 - Publish Thimble pages to the new make detail page

Status: RESOLVED FIXED

(Webmaker Graveyard :: Thimble, defect)

Description

[Brett Gaylor [:brett]]

Thimble pages should be published into the new detail page as an iframe.

Comment 1

[Scott [:thecount] Downe]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/78

Sooo, the actual code here is small.

The html page I pulled in is a big whole sale from bug 871727. Needs work to better fit into thimble.

Also, I have a bunch of css from the original that was less, that I lazily compiled over to css. Looks terrible.

I think I'll probably boil that page down into something that's much easier to digest.

So, this is in review, not because it is done, but because I want to do more in other tickets.

Comment 2

[Pomax]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

We're duplicating a lot of code, I'd like to have most of the shared code paths between the new make detail publish and the older regular published unified in functions that both middleware functions can call, with the required libraries cached at the app.js level.

Comment 3

[Scott [:thecount] Downe]

Pomax agreed with the code duplication, but the review was more for general direction, which is wrong.

We want only one page for google analytics reasons.

Comment 4

[Scott [:thecount] Downe]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

Updated it to inject instead of making a second publish.

Comment 5

[Scott [:thecount] Downe]

There is a massive css file, and a small chunk of javascript used here.

I think there is value in having those files stored on webmaker.org and having both popcorn and thimble linking to the same files.

Comment 6

[Pomax]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

Some nits in the pull request, but also a request to first make the details page generic. It currently holds a random make's information, I'd rather see placeholder texts etc. that indicate what goes in the field so it's easier to iterate on.

Comment 7

[Scott [:thecount] Downe]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

Updates.

Some placeholders added.

Comment 8

[Pomax]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

pretty nice, added the example on how to ensure code runs once jquery's available, too.

Comment 9

[Scott [:thecount] Downe]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

Should be good now.

Comment 10

[Pomax]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

comments in the pull request - also a concern about styling but that's more a follow-up issue and I will raise it after we land this.

Comment 11

[Scott [:thecount] Downe]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

Updates.

Let me know if something is wrong with the created at time. It seems to be off on my machine, off in both this new patch, and the old, which was fine yesterday. I suspect it's actually something to do with the time on my machine.

Currently waiting for an hour to roll over to test it for sure. Hopefully you get better results and this is something to do with my system. If not, I'll look into it.

Comment 12

[Scott [:thecount] Downe]

I'm actually not sure about popcorn's time to english function I pulled in. Looks like it has bugs. I don't trust it yet.

I'll go fix up my other tickets then come back to it.

Comment 13

[Pomax]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

R+, but file a followup bug to have our CSS file turned into a less file instead. It's pretty large, so it's going to be worth doing.

Comment 14

[Pomax]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

Addendum: few nits in the pull request about the time code. R+ with them fixed.

Comment 15

[Scott [:thecount] Downe]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

Throwing this back on you for one final look.

Filing a follow up ticket to search on title.

Comment 16

[Scott [:thecount] Downe]

Follow up bug 878074

Comment 17

[Pomax]

Comment on attachment 754000 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/78

yup. R+

Comment 18

[Scott [:thecount] Downe]

Staged: https://github.com/mozilla/thimble.webmaker.org/commit/a3037fa20882d7bba04a74d65c12bda7e84ec4e9