[webkit-reviews] review granted: [Bug 206034] Add support for Web Inspector pages and topic taxonomy : [Attachment 387287] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Jon Davis <jond at apple.com>'s
request for review:
Bug 206034: Add support for Web Inspector pages and topic taxonomy
https://bugs.webkit.org/show_bug.cgi?id=206034

Attachment 387287: Patch

https://bugs.webkit.org/attachment.cgi?id=387287&action=review




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 387287
  --> https://bugs.webkit.org/attachment.cgi?id=387287
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387287&action=review

r=me, with some comments.

I didn't scrutinize the logic very much, but I've seen it in action and played
around with it a lot in staging, and it all looked super great there :)

Awesome work!

> Websites/webkit.org/wp-content/plugins/web-inspector-pages.php:12
> +	       'edit_web_inspector_page' => true,

NIT: is there a reason these are indented twice?

> Websites/webkit.org/wp-content/plugins/web-inspector-pages.php:20
> +	       'read_private_web_inspector_pages' => true,

Should they also be able to `'edit_private_web_inspector_pages'` and
`'delete_private_web_inspector_pages'`?

> Websites/webkit.org/wp-content/plugins/web-inspector-pages.php:54
> +    $role->add_cap('edit_web_inspector_pages');

Do you also need to add the "singular" version of these capabilities (e.g.
`'edit_web_inspector_page'`)?

> Websites/webkit.org/wp-content/plugins/web-inspector-pages.php:80
> +	       'new_item'	       => 'New Page',

NIT: `'New Web Inspector Page'`?  Ditto for the ones below too?

> Websites/webkit.org/wp-content/plugins/web-inspector-pages.php:140
> +	       'add_new_item'		    => __('Add New Topic'),

Ditto (80)

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:12
> +    $terms = get_terms( 'web_inspector_topics', array(

Style: extra space between `( '`

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:14
> +    ) );

Ditto (12)

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:19
> +    <style>

Is there a reason that everything is indented?

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:33
> +	       background-color: hsl(0, 0%, 0%);

NIT: I would reorder most of the CSS properties to follow these "categories":

display
positioning
sizing
margin
padding
content
background
border
outline
etc.

But frankly, this isn't a huge deal, so it's up to you.  It's more important to
match the rest of webkit.org than to match my style :P

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:231
> +	       <input type="text" id="search" class="search-input"
placeholder="Search Web Inspector Reference…" title="Filter the
reference articles." required><label
class="filters-toggle-button">Filters</label>

I didn't know about `…` o.0
That's cool!

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:235
> +		   <?php endforeach;?>

Style: missing space between `;?`

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:240
> +    <?php if ( have_posts() ): ?>

Style: extra spaces around `(` and `)`

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:242
> +	       <?php while ( $query->have_posts() ) : $query->the_post();

Ditto (240)

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:252
> +	   var filtersForm = document.getElementById('reference-filters');

NIT: `let` or `const`?

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:264
> +	       if (searchTerm.length == 0 && filteredTopics.length == 0) {

NIT: `===`?

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:273
> +		       if (
ref.getElementsByTagName('ul')[0].textContent.includes(filteredTopic.value) ) {

Ditto (240)

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:282
> +		   if (searchTerm.length == 0)

Ditto (264)

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:288
> +		   else ref.classList.add('hidden');

Style: please put the body of the `else` on a new line

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:344
> +	       else inputField.placeholder = 'Search Web Inspector
Referenceâ¦';

Ditto (288)

>
Websites/webkit.org/wp-content/themes/webkit/archive-web_inspector_page.php:375
> +

Style: extra newline

> Websites/webkit.org/wp-content/themes/webkit/single-web_inspector_page.php:2
> +

Style: any reason for the extra newlines in this file?

> Websites/webkit.org/wp-content/themes/webkit/single-web_inspector_page.php:19
> +		   <p class="updated">Last updated <?php the_modified_date();
?> by <?php the_modified_author(); ?></p>

Would it be possible to include a listing of all authors to ever touch this
file?  That way someone who has a question has a better chance of reaching
someone who is familiar with the content, rather than just the last person who
edited it (which could be someone who just fixed a typo or something).