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

Please consider namespace for the styles! #1796

Closed
geminorum opened this Issue Jan 7, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@geminorum
Copy link

geminorum commented Jan 7, 2019

.tablenav.top,
.tablenav.bottom {
display: none;
}

seriously?

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 7, 2019

@geminorum Please elaborate on what the problem is. Is this causing elements on the post list table to be erroneously hidden? Please share a screenshot.

The underlying problem may be that the condition for enqueueing the stylesheet is not properly being limited to the amp_validated_url post type:

if ( 'post.php' === $pagenow ) {
wp_enqueue_style(
'amp-validation-single-error-url',
amp_get_asset_url( 'css/amp-validation-single-error-url.css' ),
array( 'common' ),
AMP__VERSION
);

@geminorum

This comment has been minimized.

Copy link
Author

geminorum commented Jan 7, 2019

@westonruter The style is based on the assumption that only List-table on post screen belongs to amp plugin. So it hides every .tablenav on the post editor. This is not a good practice to expect the end user running a site only, just to use our plugin. All additional styles should be limited to the scope of targeted elements.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 7, 2019

@geminorum I think the problem is that the stylesheet is incorrectly being enqueued on post types other than amp_validated_url. Please confirm.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 7, 2019

In other words, I think a patch like this is needed:

--- a/includes/validation/class-amp-validation-error-taxonomy.php
+++ b/includes/validation/class-amp-validation-error-taxonomy.php
@@ -867,7 +867,7 @@ class AMP_Validation_Error_Taxonomy {
 				);
 			}
 
-			if ( 'post.php' === $pagenow ) {
+			if ( 'post.php' === $pagenow && AMP_Validated_URL_Post_Type::POST_TYPE_SLUG === get_current_screen()->post_type ) {
 				wp_enqueue_style(
 					'amp-validation-single-error-url',
 					amp_get_asset_url( 'css/amp-validation-single-error-url.css' ),
@geminorum

This comment has been minimized.

Copy link
Author

geminorum commented Jan 7, 2019

confirming, the styles and the scripts are enqueued on all post-types.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 7, 2019

@geminorum Thanks. Please test #1798.

@geminorum

This comment has been minimized.

Copy link
Author

geminorum commented Jan 7, 2019

works fine.

before:
chrome_2019-01-07_22-34-05

after:
chrome_2019-01-07_22-35-11

thanks.

@westonruter westonruter added this to the v 1.0.2 milestone Jan 7, 2019

@westonruter westonruter closed this Jan 9, 2019

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.