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

Hide tooltips that should be hidden #2988

Merged
merged 1 commit into from
Aug 12, 2019
Merged

Hide tooltips that should be hidden #2988

merged 1 commit into from
Aug 12, 2019

Conversation

swissspidy
Copy link
Collaborator

Hides tooltip containers that don't seem to be actually used themselves. Only their content is extracted in amp-validation-tooltips.js.

Before:

Screenshot 2019-08-08 at 16 17 55

After:

Screenshot 2019-08-08 at 16 26 45

@swissspidy swissspidy added the Bug Something isn't working label Aug 8, 2019
@swissspidy swissspidy added this to the v1.2.1 milestone Aug 8, 2019
@swissspidy swissspidy requested a review from westonruter August 8, 2019 14:28
@googlebot googlebot added the cla: yes Signed the Google CLA label Aug 8, 2019
@westonruter
Copy link
Member

Where are you seeing these containers being shown? I don't see them at all on any of the compatibility tool admin screens.

@swissspidy
Copy link
Collaborator Author

On the Error Index screen, the Validated URLs screen, and on the Validated URLs details screen.

Example:

Screenshot 2019-08-09 at 11 29 46

Happens on my personal site as well as locally on my AMP plugin dev environment.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Humm. I don't see it on my sites. Perhaps it's due to a browser extension you have? In any case, if this PR fixes the problem for you and the tooltips still appear when clicked, then I don't see any problem with merging this.

@swissspidy
Copy link
Collaborator Author

Indeed seems to come from an injected stylesheet from an extension. I think it makes sense to explicitly hide the tooltip element to avoid such a conflict.

@swissspidy swissspidy merged commit 800560a into develop Aug 12, 2019
@swissspidy swissspidy deleted the fix/tooltip-css branch August 12, 2019 08:49
westonruter added a commit that referenced this pull request Aug 12, 2019
…p-bind-syntax

* 'develop' of github.com:ampproject/amp-wp:
  RTLCSS all the things (#2977)
  Fix AMP Story editor compatibility with code editor (#3007)
  Update dependency core-js to v3.2.1 (#3011)
  Update amphtml validator spec to v1907301630320 (#3003)
  Improve handling of unlisted Vimeo videos (#2986)
  Always hide AMP admin menu item and compatibility tool menu ite… (#3005)
  Update dependency dom-scroll-into-view to v2.0.1 (#3008)
  Hide tooltips that should be hidden (#2988)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants