Skip to content
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

New flow #400

Merged
merged 28 commits into from Aug 17, 2016

Conversation

Projects
None yet
3 participants
@diegoquinteiro
Copy link
Collaborator

commented Aug 10, 2016

This PR replaces the existing settings screen with a new layout and flow.

See screenshots:
1
2
3
4
5
6

@diegoquinteiro

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 10, 2016

Hey @philipjohn, thanks a lot for looking at this in a short timeline!

Here's the WIP PR you can comment on. I'll be finishing the last feature today and address any feedback you provide before the EOW.

@diegoquinteiro diegoquinteiro added this to the 3.1 milestone Aug 10, 2016

@diegoquinteiro diegoquinteiro changed the title [WIP] New flow New flow Aug 12, 2016

}
$new_state = filter_input( INPUT_POST, 'new_state' );
$params = filter_input( INPUT_POST, 'params' );

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

As line 112

die();
}
$app_id = filter_input( INPUT_POST, 'app_id' );

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

As 112 - let's use WP sanitisation

$pages = $fb_helper->get_pages();
if ( isset( $pages[ $page_id ] ) && $pages[ $page_id ][ 'supports_instant_articles' ] ) {
die( 'yes' );

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

This could probably be done better, but works for now

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Aug 16, 2016

Author Collaborator

It looks very hacky indeed. Do you have any specific recommendation?

<li>Create an app name, input your email and select 'Apps for Pages' in the 'Category' dropdown menu. Click 'Create App ID' when you’re ready.</li>
<li>Click on 'Settings' in the left nav bar.</li>
<li>Click '+Add Platform' at the bottom of the page and select 'Website.'</li>
<li>Under both Website and App Domains enter your domain: <b><?php echo self::get_admin_url(); ?></b></li>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

We should use esc_url() here

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Aug 16, 2016

Author Collaborator

👍

<h3>Get Started with the Instant Articles WordPress Plugin</h3>
<div class="instant-articles-card-steps">
<div class="instant-articles-card-step">
<img src="<?php echo esc_attr( plugin_dir_url( __FILE__ ) ); ?>../../assets/key@2x.png">

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_url() should be used for escaping URLs over esc_attr()

<p>If you don't have one already, set up your Facebook Developers App. Then log in to connect your plugin.</p>
</div>
<div class="instant-articles-card-step">
<img src="<?php echo esc_attr( plugin_dir_url( __FILE__ ) ); ?>../../assets/connect@2x.png">

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_url() should be used for escaping URLs over esc_attr()

<p>Select the Page you'll use to publish your Instant Articles.</p>
</div>
<div class="instant-articles-card-step">
<img src="<?php echo esc_attr( plugin_dir_url( __FILE__ ) ); ?>../../assets/customize@2x.png">

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_url() should be used for escaping URLs over esc_attr()

<p>Use our Style Editor to make your Instant Articles look just how you want them to.</p>
</div>
<div class="instant-articles-card-step">
<img src="<?php echo esc_attr( plugin_dir_url( __FILE__ ) ); ?>../../assets/check@2x.png">

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_url() should be used for escaping URLs over esc_attr()

<span class="instant-articles-card-title-checkmark">✔</span>
<label class="instant-articles-card-title-label">App connected:</label>
<span class="instant-articles-card-title-value"><?php echo esc_html( $fb_app_settings[ 'app_id' ] ); ?></span>
<a class="instant-articles-wizard-transition instant-articles-card-title-edit" href="#" data-new-state="<?php echo esc_html( Instant_Articles_Wizard_State::STATE_APP_SETUP ); ?>"></a>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_attr() rather than esc_html() should be used here

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

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

For this URL output, either the individual vars should be escaped with rawurlencode() or the whole lot with esc_url()

<span class="instant-articles-card-title-checkmark">✔</span>
<label class="instant-articles-card-title-label">App connected:</label>
<span class="instant-articles-card-title-value"><?php echo esc_html( $fb_app_settings[ 'app_id' ] ); ?></span>
<a class="instant-articles-wizard-transition instant-articles-card-title-edit" href="#" data-new-state="<?php echo esc_html( Instant_Articles_Wizard_State::STATE_APP_SETUP ); ?>"></a>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_attr() should be used here over esc_html()

<?php if ( $fb_page_settings[ 'page_picture' ] ) : ?>
<img class="instant-articles-page-img" src="<?php echo esc_attr( $fb_page_settings[ 'page_picture' ] ) ?>"/>
<?php endif; ?>
<a class="instant-articles-wizard-transition instant-articles-card-title-edit" href="#" data-new-state="<?php echo esc_html( Instant_Articles_Wizard_State::STATE_PAGE_SELECTION ); ?>"></a>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_attr() should be used here over esc_html()

<div class="instant-articles-card-title">
<h3>Style Customized</h3>
<div class="instant-articles-card-title-right">
<a class="instant-articles-wizard-transition instant-articles-card-title-edit" href="#" data-new-state="<?php echo esc_html( Instant_Articles_Wizard_State::STATE_STYLE_SELECTION ); ?>"></a>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_attr() should be used here over esc_html()

</p>
<p>
Next, set up monetization and analytics in Advanced Settings. Or explore the
<a href="https://www.facebook.com/<?php echo esc_attr( $fb_page_settings['page_id'] ); ?>/publishing_tools/?section=INSTANT_ARTICLES_SETTINGS" target="_blank">Instant Articles publishing tools</a>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_url() should be used for the whole URL here over esc_attr() for the variable

</p>
<p>
Please go to your selected
<a href="https://www.facebook.com/<?php echo esc_attr( $fb_page_settings['page_id'] ); ?>/publishing_tools/?section=INSTANT_ARTICLES_SETTINGS#Step-2" target="_blank">Facebook Page's Publishing Tools</a>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_url() should be used for the whole URL here over esc_attr() for the variable

<span class="instant-articles-card-title-checkmark">✔</span>
<label class="instant-articles-card-title-label">App connected:</label>
<span class="instant-articles-card-title-value"><?php echo esc_html( $fb_app_settings[ 'app_id' ] ); ?></span>
<a class="instant-articles-wizard-transition instant-articles-card-title-edit" href="#" data-new-state="<?php echo esc_html( Instant_Articles_Wizard_State::STATE_APP_SETUP ); ?>"></a>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_attr() should be used here rather than esc_html()

<div class="instant-articles-card-title-right">
<a href="" class="instant-articles-card-title-link"><?php echo esc_html( $fb_page_settings[ 'page_name' ] ) ?></a>
<?php if ( $fb_page_settings[ 'page_picture' ] ) : ?>
<img class="instant-articles-page-img" src="<?php echo esc_attr( $fb_page_settings[ 'page_picture' ] ) ?>"/>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_attr() should be used here rather than esc_attr()

<?php if ( $fb_page_settings[ 'page_picture' ] ) : ?>
<img class="instant-articles-page-img" src="<?php echo esc_attr( $fb_page_settings[ 'page_picture' ] ) ?>"/>
<?php endif; ?>
<a class="instant-articles-wizard-transition instant-articles-card-title-edit" href="#" data-new-state="<?php echo esc_html( Instant_Articles_Wizard_State::STATE_PAGE_SELECTION ); ?>"></a>

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_url() should be used here rather than esc_html()

</p>
<a
id="instant-articles-wizard-customize-style"
href="https://www.facebook.com/<?php echo esc_attr( $fb_page_settings['page_id'] ); ?>/publishing_tools/?section=INSTANT_ARTICLES_SETTINGS&style=<?php echo esc_attr($article_style); ?>"

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_url() should be used here for the whole URL

target="_blank">
Customize
</a>
<button id="instant-articles-wizard-customize-style-next" class="instant-articles-button instant-articles-wizard-transition" data-new-state="<?php echo esc_html( Instant_Articles_Wizard_State::STATE_REVIEW_SUBMISSION ); ?>">

This comment has been minimized.

Copy link
@philipjohn

philipjohn Aug 13, 2016

Member

esc_attr() should be used here rather than esc_html()

@philipjohn

This comment has been minimized.

Copy link
Member

commented Aug 13, 2016

Thanks @diegoquinteiro - I've left a bunch of inline comments. Mostly escaping choices that should be a quick fix. Otherwise looks good!

@diegoquinteiro diegoquinteiro merged commit ff57723 into master Aug 17, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
@diegoquinteiro

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 17, 2016

@philipjohn I've went ahead and merged after addressing the comments. Please let me know if you have new comments so we can address them in a new PR. Thanks!

@philipjohn philipjohn deleted the new_flow branch Aug 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.