Closed
Bug 888254
Opened 12 years ago
Closed 12 years ago
Links in embedded Thimble pages navigate in-frame (causing multiple make details bars, etc)
Categories
(Webmaker Graveyard :: Thimble, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: laura, Assigned: michiel)
Details
Attachments
(1 file)
STR
Go here: https://mozteach.makes.org/thimble/how-to-use-the-hacking-guides
Click on "Online Storytelling", notice the second make details bar.
Now click on "Spectrogram for Interactive Video", notice the third make details bar
Should only display the Make Details for the Make the user is looking at. Shouldn't it also take the user to the URL of whatever it is they clicked on?
If you follow these STR, look at the URL, it always stays https://mozteach.makes.org/thimble/how-to-use-the-hacking-guides
morphed to its relevant Thimble bug - we can solve this by giving the embedded page a <base> element that says that all URLs must open in a new tab/window. We might also be able to set this on the iframe rather than its content, so that pages won't need this element embedded, but that depends on whether we can access the iframe's contentWindow/contentDocument. We probably can, since we control the iframe's position on the page.
Component: webmaker.org → Thimble
Summary: Make Details on child pages and direct linking → Links in embedded Thimble pages navigate in-frame (causing multiple make details bars, etc)
also note this requires the "base" tag to be in the bleach.js whitelist, with the "href" and "target" attributes whitelisted for it, too.
Attachment #768961 -
Flags: review?(scott)
Comment 4•12 years ago
|
||
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
I'm fine with this.
Looks like jbuck has some comments though. Should probably run this by him too.
Updated•12 years ago
|
Attachment #768961 -
Flags: review?(scott) → review+
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
sanity check. This was the only nit, right?
Attachment #768961 -
Flags: review+ → review?(jon)
Comment 6•12 years ago
|
||
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
Well I'd really like you to use res.render() instead of rendering that template through the nunjucks API...
Attachment #768961 -
Flags: review?(jon) → review+
I'd much rather have the entire thing as a single nunjucks-with-inclusions file. res.render() hides too much of what it's using for its variable instantiation, and the way we're using it here, has nothing to do with the response at all, making it an inappropriate API.
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
had some odd git rebase artifacts going on, I'm pretty sure the code's the same as pre-rebase now, but I'd like to be sure here.
Attachment #768961 -
Flags: review+ → review?(scott)
Comment 9•12 years ago
|
||
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
Nothing looks out of the ordinary.
Attachment #768961 -
Flags: review?(scott) → review+
Comment 10•12 years ago
|
||
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
Actually hold up.
Not working for me anymore.
Attachment #768961 -
Flags: review+ → review?(scott)
Assignee | ||
Comment 11•12 years ago
|
||
did this work again after clearing the dbs?
Attachment #768961 -
Flags: review?(scott) → review?(chris)
Assignee | ||
Comment 12•12 years ago
|
||
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
actually, there's some errors left that need fixing. removing outstanding review requests.
Attachment #768961 -
Flags: review?(chris) → review-
Assignee | ||
Comment 13•12 years ago
|
||
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
fixed the bad filename. According to jbuck the errors yesterday came from a bad commit on the makeapi, so this should be testable either with an updated, fixed makeapi, or by revert to prior to the bad commit.
Attachment #768961 -
Flags: review- → review?(scott)
Comment 14•12 years ago
|
||
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
One line comment.
Should be good after a rebase and a fixing the comment.
Attachment #768961 -
Flags: review?(scott) → review-
Comment 15•12 years ago
|
||
Scratch that, two comments.
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
rebased, base tag placement fixed, too. You're a lifesaver overhere.
Attachment #768961 -
Flags: review- → review?(scott)
Comment 17•12 years ago
|
||
Comment on attachment 768961 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/165
All working :D
Attachment #768961 -
Flags: review?(scott) → review+
Comment 18•12 years ago
|
||
Commit pushed to master at https://github.com/mozilla/thimble.webmaker.org
https://github.com/mozilla/thimble.webmaker.org/commit/a1c62c2975426cacfac56eb37cc3cbf55ee21a20
Merge pull request #165 from Pomax/bug888254
Bug888254 - made links use _blank rather than loading in the iframe embed.
Assignee | ||
Comment 19•12 years ago
|
||
and there was much rejoicing.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in
before you can comment on or make changes to this bug.
Description
•