Use React-based component for Stylesheets box on Validated URL screen #6442

delawski · 2021-07-02T16:17:21Z

Summary

This PR replaces the server-side rendered "Stylesheet" meta box on the Validated URL screen with a React-based component fed via the REST API.

Tasks list (subject to change):

Fixes #6441

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

delawski · 2021-07-02T16:20:18Z

src/Validation/URLValidationRESTController.php

+	 * @param  WP_REST_Request $request Full details about the request.
+	 * @return true|WP_Error True if the request has permission; WP_Error object otherwise.
+	 */
+	public function get_item_permissions_check( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable


I was thinking if we could reuse and rename the create_item_permissions_check() method instead of adding a new checker just for the READABLE method. Right now, they are the same.

delawski · 2021-07-02T16:22:45Z

src/Validation/URLValidationRESTController.php

+	 */
+	public function get_validated_url( $request ) {
+		$post_id       = (int) $request['id'];
+		$validated_url = new ValidatedUrlDataProvider( $post_id );


I wasn't sure if it's the best practice to instantiate a Provider inside a method like here. Other providers are passed to the controller's __construtor. Or else, maybe the ValidatedUrlDataProvider shouldn't be a Provider in the first place?

Any use of new for something that is not a newable object (i.e. value objects, factories, ...) is not best practice, as we're switching the code over to make use of an injector across the board.

As for your question about the nature of the provider in general, I think the modeling seems to be off.

Best practice would be to have the provider be injected via the constructor, and then have a method that accepts the post ID for which it should provide.
So, following from the name, I'd expect a ValidatedUrlDataProvider to provide a ValidatedUrlData object:

$validated_url_data = $this->validated_url_data_provider->for( $post_id );

In your current design the provider is the data it provides, as it receives the post ID already in its constructor. For multiple post IDs, you'd then need not multiple data objects, but multiple provider objects.

Hope this helps.

Your feedback is very helpful, thanks @schlessera!

I've addressed the modelling problem in 5944bbc. According to your suggestion, there's a new ValidatedUrlData object that's returned by the provider.

The ValidatedUrlData object is a wrapper for the underlying WP_Post object. I'm wondering what would be the best model that would cover a potential data layer change from WP_Post to something else in the future.

delawski · 2021-07-02T16:30:28Z

src/Validation/URLValidationRESTController.php

+		$data = [
+			'id'          => $validated_url->get_id(),
+			'url'         => $validated_url->get_url(),
+			'date'        => $validated_url->get_date(),
+			'author'      => $validated_url->get_author(),
+			'stylesheets' => $validated_url->get_stylesheets(),
+		];


It's intentional that there's no single implicit $validated_url->get_data() call. The reasoning is that the REST controller is the place where the response data should be defined explicitly.

codecov · 2021-07-29T16:55:44Z

Codecov Report

Merging #6442 (b9de4d5) into develop (24923a4) will increase coverage by 0.51%.
The diff coverage is 56.80%.

❗ Current head b9de4d5 differs from pull request most recent head 26525f8. Consider uploading reports for the commit 26525f8 to get more accurate results

@@              Coverage Diff              @@
##             develop    #6442      +/-   ##
=============================================
+ Coverage      75.52%   76.04%   +0.51%     
- Complexity      5987     5999      +12     
=============================================
  Files            190      266      +76     
  Lines          18037    19242    +1205     
=============================================
+ Hits           13623    14633    +1010     
- Misses          4414     4609     +195

Flag	Coverage Δ
javascript	`71.99% <58.88%> (?)`
php	`76.30% <53.62%> (+0.78%)`	⬆️
unit	`76.30% <53.62%> (+0.78%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...validation/components/amp-document-status/index.js	`100.00% <ø> (ø)`
...nents/amp-validation-status/status-notification.js	`100.00% <ø> (ø)`
src/AmpWpPlugin.php	`100.00% <ø> (ø)`
src/Validation/URLValidationRESTController.php	`86.13% <0.00%> (-6.42%)`	⬇️
...s/src/components/plugins-context-provider/index.js	`7.40% <7.40%> (ø)`
...ts/src/components/themes-context-provider/index.js	`7.40% <7.40%> (ø)`
assets/src/components/source-details/title.js	`11.53% <11.53%> (ø)`
assets/src/components/source-details/value.js	`11.53% <11.53%> (ø)`
...components/validated-url-context-provider/index.js	`14.28% <14.28%> (ø)`
src/Validation/ValidatedUrlData.php	`18.89% <18.89%> (ø)`
... and 103 more

github-actions · 2021-07-29T17:00:25Z

Plugin builds for 26525f8 are ready 🛎️!

Download development build
Download production build

…aProvider`

delawski · 2021-08-17T12:26:09Z

The PR is now rebased onto develop and the conflicts have been resolved. Also, I've removed some bugs related to the Sources component.

…tring`

schlessera

Some minor things I just noticed...

schlessera · 2021-08-17T13:20:45Z

.phpstorm.meta.php

@@ -15,6 +15,7 @@
 			'admin.paired_browsing'              => \AmpProject\AmpWP\Admin\PairedBrowsing::class,
 			'admin.plugin_row_meta'              => \AmpProject\AmpWP\Admin\PluginRowMeta::class,
 			'admin.polyfills'                    => \AmpProject\AmpWP\Admin\Polyfills::class,
+			'admin.rest_preloader'               => \AmpProject\AmpWP\Admin\RESTPreloader::class,


This is missing the second service that was added:

'validated_url_rest_controller' => \AmpProject\AmpWP\Validation\ValidatedUrlRESTController::class,

Good catch. I've added it back in 26525f8.

src/Validation/ValidatedUrlData.php

Co-authored-by: Alain Schlesser <[email protected]>

delawski added Developer Tools REST API Issues, features related to REST API WS:UX Work stream for UX/Front-end labels Jul 2, 2021

delawski requested review from westonruter, dhaval-parekh and pierlon July 2, 2021 16:17

delawski force-pushed the add/6441-react-stylesheets-meta-box branch from 8a7b20a to a7b5c64 Compare July 2, 2021 16:31

delawski commented Jul 2, 2021

View reviewed changes

delawski force-pushed the add/6441-react-stylesheets-meta-box branch 3 times, most recently from 20c73be to 0d765cd Compare July 7, 2021 09:28

delawski marked this pull request as ready for review July 29, 2021 16:51

westonruter mentioned this pull request Jul 29, 2021

REST endpoints for AMP validation errors and validated URLs #5552

Closed

5 tasks

delawski added 15 commits August 17, 2021 11:30

Add validated-urls endpoint returning stylesheets data

7c15cc9

Introduce ValidatedUrlData that is returned by the `ValidatedUrlDat…

73568d9

…aProvider`

Add Validated URL JavaScript entry point

8aba364

Use local variable for storing current post

e43d296

Remove dead code

22aae9e

Fix and update unit tests

8824f83

Calculate stylesheets sizes and add to app context

56ef32f

Add StylesheetsSummary component

e71fbf1

Add locale-aware number formatting helper

bd11a06

Introduce FormattedMemoryValue with tests

79a4ac7

Add CSS budget-related values to stylesheets summary table

bf35648

Move icons to parent components directory

74c8732

Introduce validation status icon component

7fac19b

Add status icons to stylesheets summary table

a9e3c39

Add warning and error notices related to CSS usage

cd81f7b

delawski added 16 commits August 17, 2021 11:30

Memoize summarized sources

cf9866c

Add loading state to shaken CSS tokens diff

b9614c9

Add some spacing to shaken tokens diff toggle

e1165f6

Add Validated URL data preloading via RESTPreloader

18ffb6b

Add missing import

937fa02

Move validated-url endpoint to separate controller and add schema

4ba9b42

Remove server-side rendered Stylesheets meta box

e8087f5

Fix phpcs issues

78724d5

Clean up styles for Stylesheets meta box

658fd5a

Fix php unit test

6cd5968

Add plugins context and convert slug into plugin name

477831d

Add themes context and convert slug into theme name

cb7d331

Skip including Gutenberg in summary if there is another plugin

05361ad

Ensure property is set

a6708ff

Ensure sources prop has default value

612560c

Use default labels for unrecognized themes and plugins

0403e29

delawski force-pushed the add/6441-react-stylesheets-meta-box branch from 84fcf86 to 0403e29 Compare August 17, 2021 12:24

delawski added 2 commits August 17, 2021 15:21

Increase test coverage

9d0ec88

Replace deprecated assertStringContains with `assertStringContainsS…

d7581b5

…tring`

schlessera requested changes Aug 17, 2021

View reviewed changes

delawski and others added 2 commits August 17, 2021 15:32

Apply suggestions from code review

b9de4d5

Co-authored-by: Alain Schlesser <[email protected]>

Add missing service

26525f8

delawski requested a review from schlessera August 17, 2021 13:37

westonruter added this to the v2.2 milestone Sep 10, 2021

westonruter modified the milestones: v2.2, v2.3 Sep 28, 2021

westonruter modified the milestones: v2.3, v2.4 Dec 23, 2021

westonruter removed this from the v2.4 milestone Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use React-based component for Stylesheets box on Validated URL screen #6442

Use React-based component for Stylesheets box on Validated URL screen #6442

delawski commented Jul 2, 2021 •

edited by westonruter

Loading

delawski Jul 2, 2021

delawski Jul 2, 2021

schlessera Jul 2, 2021

delawski Jul 5, 2021

delawski Jul 2, 2021

codecov bot commented Jul 29, 2021 •

edited

Loading

github-actions bot commented Jul 29, 2021 •

edited

Loading

delawski commented Aug 17, 2021

schlessera left a comment

schlessera Aug 17, 2021

delawski Aug 17, 2021

Use React-based component for Stylesheets box on Validated URL screen #6442

Are you sure you want to change the base?

Use React-based component for Stylesheets box on Validated URL screen #6442

Conversation

delawski commented Jul 2, 2021 • edited by westonruter Loading

Summary

Checklist

delawski Jul 2, 2021

Choose a reason for hiding this comment

delawski Jul 2, 2021

Choose a reason for hiding this comment

schlessera Jul 2, 2021

Choose a reason for hiding this comment

delawski Jul 5, 2021

Choose a reason for hiding this comment

delawski Jul 2, 2021

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2021 • edited Loading

Codecov Report

github-actions bot commented Jul 29, 2021 • edited Loading

delawski commented Aug 17, 2021

schlessera left a comment

Choose a reason for hiding this comment

schlessera Aug 17, 2021

Choose a reason for hiding this comment

delawski Aug 17, 2021

Choose a reason for hiding this comment

delawski commented Jul 2, 2021 •

edited by westonruter

Loading

codecov bot commented Jul 29, 2021 •

edited

Loading

github-actions bot commented Jul 29, 2021 •

edited

Loading