Skip to content

Fix misplacement of AMP error count in editor - #5887

Merged
westonruter merged 2 commits into
developfrom
fix/amp-error-count-placement
Feb 16, 2021
Merged

Fix misplacement of AMP error count in editor#5887
westonruter merged 2 commits into
developfrom
fix/amp-error-count-placement

Conversation

@pierlon

@pierlon pierlon commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

Summary

Fixes unnoticed regression made in #5870.

Before After
image image

Checklist

  • 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).

@pierlon pierlon added Bug Something isn't working Editor WS:Core Work stream for Plugin core labels Feb 16, 2021
@pierlon pierlon added this to the v2.1 milestone Feb 16, 2021
@pierlon
pierlon requested a review from westonruter February 16, 2021 05:17
@pierlon pierlon self-assigned this Feb 16, 2021
@westonruter

westonruter commented Feb 16, 2021

Copy link
Copy Markdown
Member

Oh thanks! I noticed that too but didn't yet investigate!

@pierlon

pierlon commented Feb 16, 2021

Copy link
Copy Markdown
Contributor Author

I initially thought it was due to one of the recent dependency updates but git bisect proved its usefulness here 😄.

@github-actions

github-actions Bot commented Feb 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Plugin builds for 7455c57 are ready 🛎️!

@pierlon

pierlon commented Feb 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

That said, we may want to look into setting up Percy to aid in detecting visual regressions.

westonruter
westonruter reviewed Feb 16, 2021
View reviewed changes
Comment thread assets/src/block-validation/style.css Outdated
Comment on lines +1 to +7
.components-button[aria-label="AMP for WordPress"],
.components-button[aria-label="AMP for WordPress"]:hover,
.components-button[aria-label="AMP for WordPress"]:focus {
.components-button[aria-label="AMP"],
.components-button[aria-label="AMP"]:hover,
.components-button[aria-label="AMP"]:focus {
color: transparent;
}

.interface-pinned-items .components-button.has-icon[aria-label="AMP for WordPress"] {
.interface-pinned-items .components-button.has-icon[aria-label="AMP"] {

@westonruter westonruter Feb 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just realized that this was prone to break in the case of "AMP" being translated into another language. It was even worse before when it was "AMP for WordPress", but there is still a chance that "AMP" is localized into another language. Can we limit the selector to just .amp-toolbar-icon?

@westonruter westonruter Feb 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've tried this in 7455c57.

@pierlon pierlon Feb 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks good!

westonruter
westonruter reviewed Feb 16, 2021
View reviewed changes
Comment thread assets/src/block-validation/style.css Outdated
.components-button[aria-label="AMP"],
.components-button[aria-label="AMP"]:hover,
.components-button[aria-label="AMP"]:focus {
color: transparent;

@westonruter westonruter Feb 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see what this transparent color was being used for. I removed it in 7455c57.

@westonruter
westonruter merged commit d12bd98 into develop Feb 16, 2021
@westonruter
westonruter deleted the fix/amp-error-count-placement branch February 16, 2021 05:59
@westonruter westonruter self-assigned this Apr 25, 2021
@westonruter

westonruter commented Apr 25, 2021

Copy link
Copy Markdown
Member

QA Passed

Placement remains good:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonruter westonruter westonruter approved these changes

Assignees

@westonruter westonruter

Labels

Bug Something isn't working Editor WS:Core Work stream for Plugin core

Projects

None yet

Milestone

v2.1

Development

Successfully merging this pull request may close these issues.

2 participants

@pierlon @westonruter