Skip to content

Conversation

@zutigrm
Copy link
Collaborator

@zutigrm zutigrm commented Jan 31, 2024

Summary

Addresses issue:

Relevant technical choices

Things to note:

  • On mobile for consistency buttons remind in same position as on desktop - in design they reverse order on mobile, which is probably not intentional, but let me know if I am wrong
  • Added small and medium props to the ModalDialog component as there are 3 different modal sizes in figma, I used the widest one as default, and 2 other can be used as needed. medium is for permissions modal, where it is included now, and small should be used with audience permission modals - like this one
  • There is an autofocus happening in Modals by default (it was working like that before, no edits are done for it as it comes from the third party component), to focus both buttons in modal footer, probably for accessibility purposes. It highlights the tertiary button giving it a background. Some tweak for this should be possible, but since it was working like that before I did not tweak this, but let me know if I should.

PR Author Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 5.2 and PHP 5.6.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

  • Run the code.
  • Ensure the acceptance criteria are satisfied.
  • Reassess the implementation with the IB.
  • Ensure no unrelated changes are included.
  • Ensure CI checks pass.
  • Check Storybook where applicable.
  • Ensure there is a QA Brief.

Merge Reviewer Checklist

  • Ensure the PR has the correct target branch.
  • Double-check that the PR is okay to be merged.
  • Ensure the corresponding issue has a ZenHub release assigned.
  • Add a changelog message to the issue.

@github-actions
Copy link

github-actions bot commented Jan 31, 2024

Build files for 8a8acb8 have been deleted.

@github-actions
Copy link

github-actions bot commented Jan 31, 2024

Size Change: +16.7 kB (+1%)

Total Size: 1.47 MB

Filename Size Change
./dist/assets/css/googlesitekit-admin-css-********************.min.css 50.8 kB +384 B (+1%)
./dist/assets/js/googlesitekit-activation-********************.js 23.8 kB +217 B (+1%)
./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js 52 kB +140 B (0%)
./dist/assets/js/googlesitekit-adminbar-********************.js 34.3 kB +238 B (+1%)
./dist/assets/js/googlesitekit-api-********************.js 10 kB -1 B (0%)
./dist/assets/js/googlesitekit-components-gm2-********************.js 5.75 kB +1 B (0%)
./dist/assets/js/googlesitekit-components-gm3-********************.js 10 kB -1 B (0%)
./dist/assets/js/googlesitekit-data-********************.js 2.17 kB -11 B (-1%)
./dist/assets/js/googlesitekit-datastore-forms-********************.js 9.09 kB +4 B (0%)
./dist/assets/js/googlesitekit-datastore-site-********************.js 17.9 kB +9 B (0%)
./dist/assets/js/googlesitekit-datastore-ui-********************.js 9.22 kB +6 B (0%)
./dist/assets/js/googlesitekit-datastore-user-********************.js 23.7 kB +134 B (+1%)
./dist/assets/js/googlesitekit-entity-dashboard-********************.js 88.3 kB +2.18 kB (+3%)
./dist/assets/js/googlesitekit-main-dashboard-********************.js 107 kB +1.9 kB (+2%)
./dist/assets/js/googlesitekit-modules-********************.js 22 kB -3 B (0%)
./dist/assets/js/googlesitekit-modules-adsense-********************.js 103 kB -37 B (0%)
./dist/assets/js/googlesitekit-modules-analytics-4-********************.js 89.7 kB +10.5 kB (+13%) ⚠️
./dist/assets/js/googlesitekit-modules-analytics-********************.js 90.1 kB -272 B (0%)
./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js 22.5 kB +158 B (+1%)
./dist/assets/js/googlesitekit-modules-search-console-********************.js 59.2 kB +100 B (0%)
./dist/assets/js/googlesitekit-modules-tagmanager-********************.js 31.5 kB -38 B (0%)
./dist/assets/js/googlesitekit-polyfills-********************.js 379 B -1 B (0%)
./dist/assets/js/googlesitekit-settings-********************.js 50.6 kB +226 B (0%)
./dist/assets/js/googlesitekit-splash-********************.js 67.3 kB -108 B (0%)
./dist/assets/js/googlesitekit-user-input-********************.js 42.3 kB +135 B (0%)
./dist/assets/js/googlesitekit-vendor-********************.js 316 kB -6 B (0%)
./dist/assets/js/googlesitekit-widgets-********************.js 33.3 kB +76 B (0%)
./dist/assets/js/googlesitekit-wp-dashboard-********************.js 62.4 kB +695 B (+1%)
./dist/assets/js/runtime-********************.js 1.3 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size
./dist/assets/css/googlesitekit-adminbar-css-********************.min.css 11.1 kB
./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css 7.38 kB
./dist/assets/js/29-********************.js 2.8 kB
./dist/assets/js/30-********************.js 2.28 kB
./dist/assets/js/31-********************.js 3.72 kB
./dist/assets/js/32-********************.js 929 B
./dist/assets/js/33-********************.js 888 B
./dist/assets/js/34-********************.js 3.12 kB
./dist/assets/js/analytics-advanced-tracking-********************.js 778 B
./dist/assets/js/googlesitekit-datastore-location-********************.js 2.09 kB
./dist/assets/js/googlesitekit-i18n-********************.js 3.92 kB

compressed-size-action

Copy link
Collaborator

@nfmohit nfmohit left a comment

Choose a reason for hiding this comment

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

Very nice work here, thank you @zutigrm.

I've left some feedback, mostly nitpicky, but helps us be consistent with the existing standards.

Besides the feedback, there is another point I'd like to raise. We have this AC:

If I took a look at the linked Figma design, it looks like that dialog is using the 402px size. In that case, I think the ModalDialog components in assets/js/components/ResetButton.js & assets/js/components/UserMenu/index.js should use the small variant (402px width).

Please let me know what you think. Thanks!

@zutigrm
Copy link
Collaborator Author

zutigrm commented Feb 5, 2024

Thaks for the feedback @nfmohit . PR updated

Copy link
Collaborator

@nfmohit nfmohit left a comment

Choose a reason for hiding this comment

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

Nice work here, @zutigrm! I have left a few additional nitpicky comments. I hope you don't mind, thank you!

@zutigrm
Copy link
Collaborator Author

zutigrm commented Feb 6, 2024

Thanks @nfmohit for the feedback. PR is updated

Copy link
Collaborator

@nfmohit nfmohit left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @zutigrm! 👍

@nfmohit nfmohit merged commit 74ca48b into develop Feb 7, 2024
@nfmohit nfmohit deleted the enhancement/8110-update-modal-dialog branch February 7, 2024 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants