Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Setup Wizard copy updates #435

Merged
merged 11 commits into from Oct 11, 2016
Merged

Setup Wizard copy updates #435

merged 11 commits into from Oct 11, 2016

Conversation

demoive
Copy link
Contributor

@demoive demoive commented Sep 2, 2016

This PR:

  • Updates copy within Steps 1 & 3 of the Setup Wizard
  • Removes the "Enabled" label next to available FB Pages for selection
  • Uses CSS to store the state of the "advanced settings toggle" rather than manipulating DOM content in JS
    • The also permits the use of ▶︎ for the "closed" state to match the style of the corresponding "open" state (▼). Previously, ► was used to avoid it from being replaced with the ▶️ emoticon by the built-in WordPress styling.
    • The slideToggle() animation for opening/closing the section no longer exists

@@ -350,18 +350,27 @@ a.instant-articles-button {
font-weight: bold;
}
.instant-articles-advanced-settings {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selector now targets a

instead of an tag so text-decoration and display properties aren't necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just check my inline comments.

Copy link
Collaborator

@everton-rosario everton-rosario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've approved, just fix code style nits.

if ( text.indexOf( '►' ) !== -1 ) {
text = text.replace( '►', '▼' );
jQuery( '.instant-articles-wizard-toggle a' ).on( 'click', function () {
$advancedSettingsContainer = jQuery('.instant-articles-advanced-settings');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing is broken here.

Should be:
jQuery( '.instant-articles-advanced-settings' );

jQuery( '.instant-articles-wizard-toggle a' ).on( 'click', function () {
$advancedSettingsContainer = jQuery('.instant-articles-advanced-settings');
if ( $advancedSettingsContainer.attr('data-state') === 'closed' ) {
$advancedSettingsContainer.attr('data-state', 'opened');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review the spacing to be like this:

$var.function( 'parameter spaced', 'another param' );

@@ -350,18 +350,27 @@ a.instant-articles-button {
font-weight: bold;
}
.instant-articles-advanced-settings {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just check my inline comments.

Copy link
Contributor Author

@demoive demoive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code formatting changes made.

@demoive demoive added this to the 3.2 milestone Oct 4, 2016
Copy link
Collaborator

@everton-rosario everton-rosario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@everton-rosario
Copy link
Collaborator

@demoive Take a look on the conflicts. Probably you need to rebase it and then fix conflicts, before making it ready.

<span class="page-not-enabled">
This page has not been signed up yet.
<a href="https://www.facebook.com/instant_articles/signup?redirect_uri=<?php echo urlencode( $settings_url ) ?>&page_id=<?php echo urlencode( $page[ 'page_id' ] ) ?>">Sign Up</a>.
First, <a href="https://www.facebook.com/instant_articles/signup?redirect_uri=<?php echo urlencode( $settings_url ) ?>&page_id=<?php echo urlencode( $page[ 'page_id' ] ) ?>">sign up</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use rawurlencode() over urlencode() here

Copy link
Contributor Author

@demoive demoive Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at the difference between those two functions, and to be honest, I don't know the specs (e.g. RFC 3986) well enough to be able to determine which circumstances merits each one (if I'm faced with this again in the future). Any insight there?

I am updating the PR with your request.

@philipjohn philipjohn merged commit 9a4f6d5 into Automattic:master Oct 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants