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

Update publish popup title when project is live - Issue 1918 #2523

Merged
merged 1 commit into from Oct 21, 2017

Conversation

Projects
None yet
4 participants
@marcobeltempo
Contributor

marcobeltempo commented Oct 8, 2017

Fixes #1918
The original issue was that the publish popup title would always display "Publish your Project" regards if it was live or not.

@flukeout @humphd My solution was to reuse the existing h1 in "views/editor/publish.html" to assign the value depending on the current state of the project performed on the server side.
Unpublished title = "Publish your Project"
Published title = " Your Project is Online"

  1. created var TEXT_PUBLISHED_HEADER = strings.get("publishedHeader"); holds the string value from
    "locales/en-US/server.properties"

  2. added publishHeader to the Publisher() object in "\public\editor\scripts\ui\publisher.js". This gets the <h1> id in "views/editor/publish.html"

  3. Publisher.prototype.updateDialog located in "views/editor/publish.html" already checks the handler of the project status.

  4. If the project is published, the header gets assigned TEXT_PUBLISHED_HEADER from step 1

  5. If the project is unpublished the header is set to default

thimble_issue1918_fix_capture

@gideonthomas gideonthomas self-requested a review Oct 10, 2017

@gideonthomas

I still need to test this but nice work so far!

@@ -213,6 +213,7 @@ snippetJSChangeStyleComment=Select the element with id='alert'
# Publishing
publishHeader=Publish your Project
publisedhHeader=Your Project is Online

This comment has been minimized.

@gideonthomas

gideonthomas Oct 11, 2017

Member

since you're using this in the client files you should add this string to the client.properties file instead as per https://github.com/mozilla/thimble.mozilla.org/wiki/Localization#client-side-localization.
Also rename this to publishHeaderOnline

@humphd

This comment has been minimized.

Member

humphd commented Oct 13, 2017

@marcobeltempo before we go to land this, I need to show you how to rebase your work, since it looks like you've got a bunch of commits in here that aren't part of your patch (did you merge with master, maybe?). It's not a problem, we can fix it, but FYI, you don't need to do that while you're developing.

@marcobeltempo

This comment has been minimized.

Contributor

marcobeltempo commented Oct 13, 2017

@humphd that would be great! I made a mess somewhere down the line and a bigger one trying to fix it.

@humphd

This comment has been minimized.

Member

humphd commented Oct 13, 2017

Yeah, it's easily fixed, don't worry. For now, just keep getting your PR fixed. Then I'll give you a hand. Remind me please, I'll forget, but I need to cover this with all of you.

@gideonthomas

gideonthomas requested changes Oct 13, 2017 edited

Good work so far @marcobeltempo!

I'm not sure how you managed to get it to revert back to "Publish this project" when you unpublish the project as I don't see code to handle that case. I added comments to suggest where to make those changes. Let me know if you're having trouble with getting it to work and please do rebase your changes before pushing them up.

@@ -27,6 +27,7 @@ renameProjectSavingIndicator=Saving…
publishPublishingIndicator=Publishing…
publishUnpublishingIndicator=Unpublishing…
publishDeleteWarning=Are you sure? The current link will stop working and if you publish again, the link will be different.
publishHeaderOnline=Your Project is Online

This comment has been minimized.

@gideonthomas

gideonthomas Oct 13, 2017

Member

also move the publishHeader string from server.properties to server-client-shared.properties since we are going to use that string in both the server and the client.

@@ -14,6 +14,7 @@ var TEXT_UNPUBLISH = strings.get("publishDeleteBtn");
var TEXT_UNPUBLISHING = strings.get("publishUnpublishingIndicator");
var TEXT_UPDATE_PUBLISH = strings.get("publishChangesBtn");
var TEXT_UNPUBLISH_WARNING = strings.get("publishDeleteWarning");
var TEXT_PUBLISHED_HEADER = strings.get("publishHeaderOnline");

This comment has been minimized.

@gideonthomas

gideonthomas Oct 13, 2017

Member

let's call the TEXT_PUBLISH_HEADER_ONLINE
and add another variable here called TEXT_PUBLISH_HEADER that stores the publishHeader string

@@ -319,6 +321,7 @@ Publisher.prototype.updateDialog = function(publishUrl, allowUnpublish) {
// "publish"/"cancel" buttons
if (allowUnpublish) {
this.dialog.buttons.parent.addClass("hide");
this.dialog.publishHeader.text(TEXT_PUBLISHED_HEADER);

This comment has been minimized.

@gideonthomas

gideonthomas Oct 13, 2017

Member

you also need to set the title in the else condition back to TEXT_PUBLISH_HEADER

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

This comment has been minimized.

@gideonthomas

gideonthomas Oct 13, 2017

Member

revert this change

@marcobeltempo

This comment has been minimized.

Contributor

marcobeltempo commented Oct 13, 2017

@gideonthomas Thank you for all your feedback! I've got it working again on my localhost. I'll commit shortly with the revisions

@flukeout

This comment has been minimized.

Contributor

flukeout commented Oct 20, 2017

Looking forward to testing this. Please ping me when it's ready @marcobeltempo!

@marcobeltempo

This comment has been minimized.

Contributor

marcobeltempo commented Oct 20, 2017

Everything's ready for testing @flukeout

@gideonthomas

This comment has been minimized.

Member

gideonthomas commented Oct 20, 2017

@marcobeltempo can you rebase this PR onto master? You seem to have some extra commits in here.

@marcobeltempo

This comment has been minimized.

Contributor

marcobeltempo commented Oct 20, 2017

@gideonthomas I rebased the PR onto master

@gideonthomas

This comment has been minimized.

Member

gideonthomas commented Oct 21, 2017

This looks great! Thanks for the contribution @marcobeltempo!

@gideonthomas gideonthomas merged commit 53a1dc4 into mozilla:master Oct 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marcobeltempo

This comment has been minimized.

Contributor

marcobeltempo commented Oct 21, 2017

@gideonthomas Thanks again for all your help! 😃

@marcobeltempo marcobeltempo deleted the marcobeltempo:issue1918 branch Oct 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment