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

For the 'Preview AMP' button, replace tooltip with title attribute #4601

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Apr 17, 2020

Summary

This fixes an issue where the tooltip was cut off:

tooltip-here

Now, the <Button> has a title attribute, instead of being wrapped in a <Tooltip>:

rep-title-attr

Fixes #4590

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

Prevents an issue where the tooltip
is cut off.
@googlebot googlebot added the cla: yes Signed the Google CLA label Apr 17, 2020
@kienstra
Copy link
Contributor Author

Should this be to the develop branch, or 1.5?

<Button
className="amp-editor-post-preview"
href={ href }
title={ __( 'Preview AMP', 'amp' ) }
Copy link
Contributor Author

@kienstra kienstra Apr 17, 2020

Choose a reason for hiding this comment

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

The label attribute is changed to title.

But other than that, this diff is just from indentation. There's no other change to <Button>

@westonruter
Copy link
Member

Develop branch, yes

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.

That works! Thanks.

@westonruter westonruter merged commit 148ad9b into develop Apr 17, 2020
@westonruter westonruter deleted the fix/amp-preview-tooltip branch April 17, 2020 04:26
westonruter pushed a commit that referenced this pull request Apr 17, 2020
Prevents an issue where the tooltip is cut off.
@kienstra
Copy link
Contributor Author

Nice, thanks!

@swissspidy
Copy link
Collaborator

Why was this particular tooltip cut off, but all other tooltips in that area work just fine?

@westonruter
Copy link
Member

Because Gutenberg was using some slot-fill mechanism to locate the tooltip element elsewhere in the DOM so that it could be positioned over the element without other DOM elements overlapping. This wasn't the case for the AMP preview button.

westonruter added a commit that referenced this pull request Apr 18, 2020
…phtml-2004041903580

* 'develop' of github.com:ampproject/amp-wp: (48 commits)
  Bump https-proxy-agent from 2.2.2 to 2.2.4 (#4596)
  Update dependency babel-jest to v25.3.0 (#4550)
  Update dependency core-js to v3.6.5 (#4558)
  For the 'Preview AMP' button, use a title instead of a tooltip (#4601)
  Bump stable tag to 1.5.3
  Fix handling of Mustache templates (#4583)
  Stub request based on test scenario (#4588)
  Return early instead of storing eventual return value in variable
  Improve phpdoc and logic conditions
  Update links in pull request template
  Update contributing.md with link to wiki
  Remove engineering.md now that it is on the wiki
  Remove project-management.md since only applicable to Stories
  Add conditions for comment feed, trackback, robots, and favicon
  Fix typo in global phpdoc
  Update tests after block-library/style.css changes in Gutenberg 7.9 (#4579)
  Remove special conditions for Reader mode; remove need for $exit condition in redirects
  Fix translators comment
  Add comment explaining short-circuit behavior when query var is present
  Update version for _doing_it_wrong() from 1.5.3 to 1.6.0
  ...
westonruter added a commit that referenced this pull request Apr 20, 2020
…filter-list-table-row-actions

* 'develop' of github.com:ampproject/amp-wp: (56 commits)
  Bump https-proxy-agent from 2.2.2 to 2.2.4 (#4596)
  Update dependency babel-jest to v25.3.0 (#4550)
  Update dependency core-js to v3.6.5 (#4558)
  For the 'Preview AMP' button, use a title instead of a tooltip (#4601)
  Update pull request template based on new workflow
  Bump stable tag to 1.5.3
  Fix handling of Mustache templates (#4583)
  Stub request based on test scenario (#4588)
  Return early instead of storing eventual return value in variable
  Improve phpdoc and logic conditions
  Update links in pull request template
  Update contributing.md with link to wiki
  Remove engineering.md now that it is on the wiki
  Remove project-management.md since only applicable to Stories
  Add conditions for comment feed, trackback, robots, and favicon
  Fix typo in global phpdoc
  Update tests after block-library/style.css changes in Gutenberg 7.9 (#4579)
  Remove special conditions for Reader mode; remove need for $exit condition in redirects
  Fix translators comment
  Add comment explaining short-circuit behavior when query var is present
  ...
westonruter added a commit that referenced this pull request Apr 21, 2020
…widgets-registration

* 'develop' of github.com:ampproject/amp-wp: (88 commits)
  Fix grammar typo
  Bump CSS cache version
  Update composer.lock
  Use patch file instead of diff
  Update patch: Fix parsing CSS selectors which contain commas
  Update php-css-parser to dev-master#bc6ec74; remove patches/php-css-parser-138-extended.patch
  Add test to demonstrate failure to parse class names containing escaped fractions
  Restrict metaboxes which appear on the validated URL screen
  Update hook priority in test_add_admin_hooks
  Restrict row actions for taxonomy terms
  Add test for disable-inline-width on amp-img
  Exclude data-ampdevmode attribute exclusion rule
  Update spec to 2004142326360 to remove container layout from amp-list
  Bump https-proxy-agent from 2.2.2 to 2.2.4 (#4596)
  Update dependency babel-jest to v25.3.0 (#4550)
  Update dependency core-js to v3.6.5 (#4558)
  For the 'Preview AMP' button, use a title instead of a tooltip (#4601)
  Update pull request template based on new workflow
  Bump stable tag to 1.5.3
  Fix handling of Mustache templates (#4583)
  ...
@westonruter westonruter added this to the v1.5.4 milestone Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preview AMP tooltip displays incorrectly in editor
4 participants