Adding embed snippet to Thimble publishing dialog Error #1543 #2535

Fatehsandhu · 2017-10-20T16:50:36Z

Fixing issue #1543 , Can I get some feedback please

modified: publish.html

gideonthomas

Thanks for getting started on this @Fatehsandhu, I added a couple of comments.

I assume you want to get the content in first before styling it as shown in the issue?

gideonthomas · 2017-10-20T21:18:51Z

scripts/setup-services.sh

@@ -83,7 +83,7 @@ echo "Setting up login.webmaker.org"
 cd login.webmaker.org
 cp env.sample .env
 sudo npm install --no-bin-links --loglevel=error # sudo needed for bcrypt permissions
-cd ..


you def need this back

gideonthomas · 2017-10-20T21:19:13Z

scripts/setup-services.sh

@@ -83,7 +83,7 @@ echo "Setting up login.webmaker.org"
 cd login.webmaker.org
 cp env.sample .env
 sudo npm install --no-bin-links --loglevel=error # sudo needed for bcrypt permissions
-cd ..
+sudo npm install --no-bin-links || sudo npm install --no-bin-links


let's remove this and file as a follow-up issue.

gideonthomas · 2017-10-20T21:19:44Z

locales/en-GB/server.properties

@@ -94,6 +94,7 @@ navToggleAutoUpdateLabel=Auto
 # Publishing
 publishHeader=Publish your Project
 publishShareLink=Here's a link you can share...
+publishEmbedLinkTitle=Copy this code into your HTML page to show off your project.


make this change in locales/en-US not locales/en-GB

gideonthomas · 2017-10-20T21:20:23Z

views/editor/publish.html

 <p>{{ gettext("publishShareLink") }}</p>
 <div id="publish-link">
 <a id="link-publish-link" title="{{ gettext("publishShareLinkTitle") }}" target="_blank" href="test"></a>
 </div>
+


remove the trailing spaces

gideonthomas · 2017-10-20T21:20:34Z

views/editor/publish.html

@@ -16,10 +16,17 @@
 </div>

 <div id="publish-live" class="hide">
+


remove the trailing spaces

gideonthomas · 2017-10-20T21:21:15Z

views/editor/publish.html

 <p>{{ gettext("publishShareLink") }}</p>
 <div id="publish-link">
 <a id="link-publish-link" title="{{ gettext("publishShareLinkTitle") }}" target="_blank" href="test"></a>
 </div>
+
+ <!-- Embed iFrame -->


remove the extra leading spaces before the comment so that it aligns with the tag

gideonthomas · 2017-10-20T21:23:25Z

views/editor/publish.html

 </div>
-
+ 


undo this change

gideonthomas · 2017-10-20T21:23:39Z

views/editor/publish.html

@@ -29,9 +36,9 @@

 <div id="publish-details">
 <label>{{ gettext("publishProjectDescription") }}</label>
- <textarea class="publish-description" placeholder="{{ gettext("publishProjectDescriptionPlaceholder") }}"></textarea>
+ <textarea class="publish-description" placeholder="{{ gettext("publishProjectDescriptionPlaceholder") }}"></textarea> 


undo this change

gideonthomas · 2017-10-20T21:33:26Z

views/editor/publish.html

+
+ <!-- Embed iFrame -->
+ <p>{{ gettext("publishEmbedLinkTitle") }}</p>
+ <div id="publish-link">


you cannot use the same id here as another element. Change this to publish-embed.

gideonthomas · 2017-10-20T21:44:18Z

views/editor/publish.html

+ <!-- Embed iFrame -->
+ <p>{{ gettext("publishEmbedLinkTitle") }}</p>
+ <div id="publish-link">
+ <span>&lt;iframe {{ gettext("PUBLISHED_PROJECTS_HOSTNAME") }} &gt; </span>


I'm not sure why "PUBLISHED_PROJECTS_HOSTNAME" would be used here.
Instead, leave the text empty and dynamically change the text with JS instead:

Add a property here called embed that is a jquery object linking to this span.

After this line set the text of embed to be the string `<iframe src={$publishUrl}></iframe>`

Fatehsandhu · 2017-10-24T18:21:14Z

@gideonthomas thank you, much appreciated. I will get on those ASAP.

…la.org

Fatehsandhu · 2017-11-22T16:24:17Z

@gideonthomas My code isn't compiling for some reason, can I please get some help?
thanks

humphd · 2017-11-22T16:47:36Z

public/editor/scripts/ui/publisher.js

@@ -33,10 +33,11 @@ function Publisher() {
 update: $("#publish-button-update"),
 unpublish: $("#publish-button-unpublish"),
 parent: $("#publish-buttons"),
- indexMessage: $("#no-index")
+ indexMessage: $("#no-index"),


You have a stray comma here. The last element in a list like this shouldn't get a dangling comma.

humphd · 2017-11-22T16:48:25Z

public/editor/scripts/ui/publisher.js

@@ -313,7 +314,8 @@ Publisher.prototype.updateDialog = function(publishUrl, allowUnpublish) {
 var published = this.dialog.published;
 var unpublishBtn = this.dialog.buttons.unpublish;
 var unpublish = this.handlers.unpublish;
-
+


It's unhappy with this line for some reason. What happens if you remove this blank line?

humphd · 2017-11-22T16:48:43Z

/home/travis/build/mozilla/thimble.mozilla.org/public/editor/scripts/ui/publisher.js

   36:35  error  Delete `,`     prettier/prettier

  317:1   error  Delete `····`  prettier/prettier

✖ 2 problems (2 errors, 0 warnings)

  2 errors, 0 warnings potentially fixable with the `--fix` option.

I left some comments in the code. These are styling issues.

Fatehsandhu · 2017-11-22T16:54:19Z

@humphd Thanks a lot for the help

gideonthomas

this needs a little bit more work @Fatehsandhu, but it's getting there.

gideonthomas · 2017-11-23T19:34:58Z

scripts/setup-services.sh

@@ -84,6 +84,7 @@ cd login.webmaker.org
 cp env.sample .env
 sudo npm install --no-bin-links --loglevel=error # sudo needed for bcrypt permissions
 cd ..
+sudo npm install --no-bin-links || sudo npm install --no-bin-links


@Fatehsandhu, please undo this change

gideonthomas · 2017-11-23T19:38:06Z

public/editor/scripts/ui/publisher.js

@@ -313,7 +314,7 @@ Publisher.prototype.updateDialog = function(publishUrl, allowUnpublish) {
 var published = this.dialog.published;
 var unpublishBtn = this.dialog.buttons.unpublish;
 var unpublish = this.handlers.unpublish;
-
+ published.embed.text("<iframe src={$publishUrl}></iframe>");


this needs to be:

published.embed.text(`<iframe src=${publishUrl}></iframe>`);

Note the use of ` instead of "

gideonthomas · 2017-11-23T19:39:06Z

views/editor/publish.html

@@ -41,4 +46,4 @@ <h1>{{ gettext("publishHeader") }}</h1>
 <div id="publish-button-publish">{{ gettext("publishBtn") }}</div>
 </div>
 </div>
-{% endblock %}
+{% endblock %}


blank line after this

gideonthomas · 2017-11-23T19:40:04Z

public/editor/scripts/ui/publisher.js

@@ -36,7 +36,8 @@ function Publisher() {
 indexMessage: $("#no-index")
 },
 description: $("#publish-details > textarea.publish-description"),
- publishHeader: $(".content > h1"),
+ embed: $("#link-publish-embed"),
+


remove this blank line

gideonthomas · 2017-11-23T19:40:56Z

views/editor/publish.html

+
+ <p>{{ gettext("publishEmbedLinkTitle") }}</p>
+ <div id="link-publish-embed">
+ <span id="link-publish-link"></span>


not sure why you need this span, you don't use it anywhere.

gideonthomas · 2017-11-24T01:30:01Z

@Fatehsandhu, can you rebase your branch onto master?

gideonthomas · 2018-01-03T01:24:52Z

@Fatehsandhu, it looks like the rebase didn't work correctly. I still see commits from master in this PR. Would you be able to only cherry-pick your commits onto master?

Fatehsandhu · 2018-01-03T01:26:45Z

@gideonthomas I am not sure how to do that correctly.

gideonthomas · 2018-01-03T04:17:41Z

Hmm, I'm not actually sure how this branch got into this state since it looks like none of your code is in this branch anymore. Maybe @humphd might be able to help you here.

humphd · 2018-01-04T17:24:00Z

@Fatehsandhu can you tell me which of the 29 commits in here contain your fix? If I know that, I can guide you through the process to fix this.

Fatehsandhu · 2018-01-04T17:29:25Z

@humphd commit #25abc0f was good.

humphd · 2018-01-04T17:31:53Z

@Fatehsandhu I'm confused, you're saying 25abc0f is your fix? That seems wrong to me. Am I misunderstanding?

Fatehsandhu · 2018-01-05T01:44:18Z

@humphd No, sorry I meant to say that I had made all the changes up until that commit. Sorry, I don't have all the changes in one pull request.

Fatehsandhu added 8 commits October 13, 2017 15:06

First working code

1d88c7a

modified: ../../locales/en-GB/server.properties

2282100

modified: publish.html

Update Vagrantfile

a0bbb83

Update publish.html

73aa025

Update publish.html

8d0f366

Update publish.html

ed90fa6

Update publish.html

4100881

Update publish.html

d1214cf

gideonthomas suggested changes Oct 20, 2017

View reviewed changes

Fatehsandhu added 9 commits November 22, 2017 10:20

Jquery not working need help

830358e

Jquery not working help

1b8f3f5

Merge branch 'master' into master

e2e9c3d

Merge branch 'master' into master

ca4b149

Jquery not working help

f9bddb8

Merge branch 'master' of https://github.com/Fatehsandhu/thimble.mozil…

e6724ea

…la.org

Jquery not working help

a4c0413

Jquery not working help

8b81b0d

Jquery not working help

9e0a749

humphd reviewed Nov 22, 2017

View reviewed changes

Jquery not working help

36786b2

gideonthomas suggested changes Nov 23, 2017

View reviewed changes

Fatehsandhu added 4 commits November 23, 2017 16:23

Update publisher.js

c1ab81e

Update publisher.js

5c5519d

Update setup-services.sh

25abc0f

Update publish.html

1aaec7a

Fatehsandhu added 7 commits December 31, 2017 21:52

Update publisher.js

901b84f

Update publish.html

02c3fb4

Update server.properties

0428408

New

66572fa

New

f6b2c1f

Update server.properties

94d5557

Update server.properties

a4c44e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding embed snippet to Thimble publishing dialog Error #1543 #2535

Adding embed snippet to Thimble publishing dialog Error #1543 #2535

Fatehsandhu commented Oct 20, 2017

gideonthomas left a comment

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

Fatehsandhu commented Oct 24, 2017

Fatehsandhu commented Nov 22, 2017 •

edited

humphd Nov 22, 2017

humphd Nov 22, 2017

humphd commented Nov 22, 2017

Fatehsandhu commented Nov 22, 2017

gideonthomas left a comment

gideonthomas Nov 23, 2017

gideonthomas Nov 23, 2017

gideonthomas Nov 23, 2017

gideonthomas Nov 23, 2017

gideonthomas Nov 23, 2017

gideonthomas commented Nov 24, 2017

gideonthomas commented Jan 3, 2018

Fatehsandhu commented Jan 3, 2018

gideonthomas commented Jan 3, 2018

humphd commented Jan 4, 2018

Fatehsandhu commented Jan 4, 2018

humphd commented Jan 4, 2018

Fatehsandhu commented Jan 5, 2018

		@@ -16,10 +16,17 @@
		</div>

		<div id="publish-live" class="hide">

Adding embed snippet to Thimble publishing dialog Error #1543 #2535

Are you sure you want to change the base?

Adding embed snippet to Thimble publishing dialog Error #1543 #2535

Conversation

Fatehsandhu commented Oct 20, 2017

gideonthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fatehsandhu commented Oct 24, 2017

Fatehsandhu commented Nov 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd commented Nov 22, 2017

Fatehsandhu commented Nov 22, 2017

gideonthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gideonthomas commented Nov 24, 2017

gideonthomas commented Jan 3, 2018

Fatehsandhu commented Jan 3, 2018

gideonthomas commented Jan 3, 2018

humphd commented Jan 4, 2018

Fatehsandhu commented Jan 4, 2018

humphd commented Jan 4, 2018

Fatehsandhu commented Jan 5, 2018

Fatehsandhu commented Nov 22, 2017 •

edited