Bug 206034 – Add support for Web Inspector pages and topic taxonomy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Website

[Jon Davis]

Adds the notion of a Web Inspector page custom post type along with topic taxonomy for maintaining, managing, and displaying Web Inspector reference article pages.

[Jon Davis]

Created attachment 387275 [details]
Patch

[Jon Davis]

Created attachment 387287 [details]
Patch

[Devin Rousso]

Comment on attachment 387287 [details]
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&hellip;" title="Filter the reference articles." required><label class="filters-toggle-button">Filters</label>

I didn't know about `&hellip;` 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).

[Jon Davis]

Comment on attachment 387287 [details]
Patch

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

>> 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'`)?

Yes

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

This will take some significant work outside the scope of this patch. I filed https://bugs.webkit.org/show_bug.cgi?id=206176

[Jon Davis]

Created attachment 387531 [details]
Patch

[Devin Rousso]

Comment on attachment 387531 [details]
Patch

r=me, nice work! :)

[WebKit Commit Bot]

Comment on attachment 387531 [details]
Patch

Clearing flags on attachment: 387531

Committed r254488: <https://trac.webkit.org/changeset/254488>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58555727>