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

Add new icon, text, and style to splash notice #1470

Merged
merged 5 commits into from Sep 27, 2018

Conversation

Projects
None yet
2 participants
@jacobschweitzer
Copy link
Contributor

jacobschweitzer commented Sep 26, 2018

No description provided.

@jacobschweitzer jacobschweitzer referenced this pull request Sep 26, 2018

Closed

Implement settings splash notice #1381

6 of 6 tasks complete

@jacobschweitzer jacobschweitzer requested a review from westonruter Sep 26, 2018

<div class="amp-welcome-notice notice notice-info is-dismissible" id="<?php echo esc_attr( $notice_id ); ?>">
<div class="notice-dismiss"></div>
<div class="amp-welcome-icon-holder">
<span class="amp-welcome-icon"></span>

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

Why not use an img here?

This comment has been minimized.

Copy link
@jacobschweitzer

jacobschweitzer Sep 27, 2018

Author Contributor

It didn't work at first, I had to change the viewBox on the SVG since there was a bunch of whitespace around it. So I ended up leaving it like this.. switching to a an img now.

<h1><?php esc_html_e( 'Welcome to AMP for WordPress', 'amp' ); ?></h1>
<h3><?php esc_html_e( 'Bring the speed and features of the open source AMP project to your site, complete with the tools to support content authoring and website development.', 'amp' ); ?></h3>
<h3><?php esc_html_e( 'From granular controls that help you create AMP content, to Core Gutenberg support, to a sanitizer that only shows visitors error-free pages, to a full error workflow for developers, this release enables rich, performant experiences for your WordPress site.', 'amp' ); ?></h3>
<a href="https://www.ampproject.org/docs/getting_started/" target="_blank" class="button button-primary">Learn More</a>

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

“Learn More” needs to be translatable.

wp_enqueue_style(
'amp-settings',
amp_get_asset_url( 'css/amp-settings.css' ),

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

Since all of this is only for the welcome notice, I think you might as well just put this into a style element right with the div.amp-welcome-notice element. Once the welcome notice is dismissed, there will not be any need to have this CSS anymore. Having a separate CSS file enqueued for this page seems a bit overkill.

This comment has been minimized.

Copy link
@jacobschweitzer

jacobschweitzer Sep 27, 2018

Author Contributor

That's how I had it at first but I changed it before creating the PR thinking that maybe later on we might want to add more styles to the settings page in general. Right now you are right, it doesn't make sense to add another enqueue method and file.

<div class="amp-welcome-notice notice notice-info is-dismissible" id="<?php echo esc_attr( $notice_id ); ?>">
<div class="notice-dismiss"></div>
<div class="amp-welcome-icon-holder">
<img class="amp-welcome-icon" src="<?php echo esc_url( amp_get_asset_url( 'images/amp-welcome-icon.svg' ) ); ?>" srcset="<?php echo esc_url( amp_get_asset_url( 'images/amp-welcome-icon.svg' ) ); ?>" />

This comment has been minimized.

Copy link
@westonruter

westonruter Sep 27, 2018

Member

Why srcset?

This comment has been minimized.

Copy link
@jacobschweitzer

jacobschweitzer Sep 27, 2018

Author Contributor

Leftover from a thought I had about adding a .png fallback. Thanks for catching this!

@westonruter westonruter added this to the v1.0 milestone Sep 27, 2018

@westonruter westonruter merged commit 726522b into develop Sep 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/1381-splash-notice-image-text branch Sep 27, 2018

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.