Bug 206034 - Add support for Web Inspector pages and topic taxonomy
Summary: Add support for Web Inspector pages and topic taxonomy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Website (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jon Davis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-09 14:10 PST by Jon Davis
Modified: 2020-01-13 20:38 PST (History)
4 users (show)

See Also:


Attachments
Patch (23.80 KB, patch)
2020-01-09 14:44 PST, Jon Davis
no flags Details | Formatted Diff | Diff
Patch (23.54 KB, patch)
2020-01-09 16:25 PST, Jon Davis
no flags Details | Formatted Diff | Diff
Patch (23.51 KB, patch)
2020-01-13 08:18 PST, Jon Davis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Davis 2020-01-09 14:10:29 PST
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.
Comment 1 Jon Davis 2020-01-09 14:44:04 PST
Created attachment 387275 [details]
Patch
Comment 2 Jon Davis 2020-01-09 16:25:37 PST
Created attachment 387287 [details]
Patch
Comment 3 Devin Rousso 2020-01-10 12:51:35 PST
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).
Comment 4 Jon Davis 2020-01-13 08:13:41 PST
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
Comment 5 Jon Davis 2020-01-13 08:18:19 PST
Created attachment 387531 [details]
Patch
Comment 6 Devin Rousso 2020-01-13 09:11:51 PST
Comment on attachment 387531 [details]
Patch

r=me, nice work! :)
Comment 7 WebKit Commit Bot 2020-01-13 20:37:31 PST
Comment on attachment 387531 [details]
Patch

Clearing flags on attachment: 387531

Committed r254488: <https://trac.webkit.org/changeset/254488>
Comment 8 WebKit Commit Bot 2020-01-13 20:37:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2020-01-13 20:38:17 PST
<rdar://problem/58555727>