Summary: | Add support for Web Inspector pages and topic taxonomy | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon Davis <jond> | ||||||||
Component: | WebKit Website | Assignee: | Jon Davis <jond> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, hi, jond, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Jon Davis
2020-01-09 14:10:29 PST
Created attachment 387275 [details]
Patch
Created attachment 387287 [details]
Patch
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…" 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). 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 Created attachment 387531 [details]
Patch
Comment on attachment 387531 [details]
Patch
r=me, nice work! :)
Comment on attachment 387531 [details] Patch Clearing flags on attachment: 387531 Committed r254488: <https://trac.webkit.org/changeset/254488> All reviewed patches have been landed. Closing bug. |