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

Restore notice max-width in setup and settings #7140

Closed
aaemnnosttv opened this issue Jun 6, 2023 · 6 comments
Closed

Restore notice max-width in setup and settings #7140

aaemnnosttv opened this issue Jun 6, 2023 · 6 comments
Labels
P1 Medium priority Type: Bug Something isn't working

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jun 6, 2023

Bug Description

In #6956 we made a change to the styling of SettingsNotice to address an inconsistency with the design when used on the dashboard, but this introduced an unintended change in the other usages in setup and settings contexts where the notice streches full-width where it should be constrained as before.

Screenshots

Before
image

Currently
image


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The max-width of inline notices (as seen above) that are not used on the SK dashboard should be consistent with their pre 1.102.0 styling and a max-width of 1098px

Implementation Brief

Test Coverage

  • No new tests needed, but VRTs may need updating.

QA Brief

  • Verify that the notices shown in the Site Kit settings and setup screens are not full-width, they should have a max-width of 1098 pixels.
  • Verify that notices shown in the Site Kit dashboard, e.g. Adsense Top Earning Pages widget with Analytics setup with GA4 only and ga4Reporting feature flag are full-width and do not have a max-width.

Changelog entry

  • Restore notice width in setup and settings views.
@aaemnnosttv aaemnnosttv added Type: Bug Something isn't working P1 Medium priority labels Jun 6, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jun 6, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Jun 6, 2023

This should be a quick one to implement and QA, so marking it as a 3 estimate.

@aaemnnosttv
Copy link
Collaborator Author

@tofumatt would this style be scoped to settings & setup views only? We wouldn't want to impose the same limitation on the dashboard or we'd be undoing the intentional change we made there :)

@tofumatt
Copy link
Collaborator

tofumatt commented Jun 6, 2023

@aaemnnosttv Argh, yes, you're totally right. I even did that when testing locally but forgot to include it in the IB. I think the provided selectors make sense, they're what I used when I tried it locally.

@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt Jun 6, 2023
@techanvil
Copy link
Collaborator

I just want to comment to say, I personally feel like this is something we'd be estimating as a 3 before the points experiment, as we had agreed a 1 would only be for super trivial things - this seems a bit more with multiple component instances to check and possible VRT updates etc.

Just chipping in because I think we need to be careful not to slip back toward our old estimations having put the experimental phase behind us...

@aaemnnosttv
Copy link
Collaborator Author

Thanks @techanvil – SGTM, I would think this one should be doable as estimated even with VRT, but I agree that it doesn't leave much room for potential complications; 7 it is!

IB ✅

@aaemnnosttv aaemnnosttv removed their assignment Jun 7, 2023
@nfmohit nfmohit self-assigned this Jun 13, 2023
@nfmohit nfmohit removed their assignment Jun 14, 2023
@aaemnnosttv aaemnnosttv self-assigned this Jun 16, 2023
@aaemnnosttv aaemnnosttv removed their assignment Jun 16, 2023
@wpdarren wpdarren self-assigned this Jun 19, 2023
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • The notices shown in the settings and setup screens are not full-width, and have a max-width of 1098 pixels.
  • Notices shown in the dashboard, e.g. Adsense Top Earning Pages widget with Analytics setup are full-width.
  • Tested to make sure there are no UI/UX issues with the notices mentioned above on mobile devices.
Screenshots

image
image
image
image
image
image
image
image
image

@wpdarren wpdarren removed their assignment Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants