New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply new error taxonomy page design #1394

Merged
merged 32 commits into from Sep 13, 2018

Conversation

Projects
None yet
3 participants
@kienstra
Collaborator

kienstra commented Sep 3, 2018

  • Reorders columns based on this design
  • Changes the heading names of the columns, and changes the date to be the time ago, like '4 days ago'

changed-amp-validation-errors

Some other changes are applied in PR #1373, like adding the filters, and changing the page <h1> from 'AMP Validation Errors' to 'Errors by Type.'

Closes #1361

Reorganize error taxonomy page columns based on design
Mainly using:
https://sketch.cloud/s/PoWy8/all/page-1/amp-validation-errors
Change the heading names of these,
and change the date to be the time ago.

@kienstra kienstra referenced this pull request Sep 3, 2018

Closed

Implement improved error listing view #1361

0 of 21 tasks complete

kienstra and others added some commits Sep 5, 2018

Merge in develop, resolve trivial conflict.
The conflict was in:
test-class-amp-validation-error-taxonomy.php
Retain both edits, just on different lines.
@johnwatkins0

This comment has been minimized.

Contributor

johnwatkins0 commented Sep 10, 2018

I've added several commits to this PR continuing @kienstra 's work for #1361. The main changes are:

  1. Apply styles and formatting for the details column (but I didn't spend too much time on it after this comment from Weston suggesting the implementation shown in the design might not be final).
  2. Add the "Error Type" column
  3. Link each term name to the post type listing with posts having the term
  4. Update "Accept", "Reject", etc. text based on AC in #1361.
  5. Implement arrow icon on the details summaries and an icon in the details column header/footer to toggle all details on the page.

Still to do:

  1. Open questions for @postphotos on #1361
  2. Status icons (dependent on PR #1397)
  3. Status and Details tooltips
public static function get_reader_friendly_error_type_text( $error_type ) {
switch ( $error_type ) {
case 'js_error':
return esc_html__( 'Javascript', 'amp' );

This comment has been minimized.

@kienstra

kienstra Sep 12, 2018

Collaborator

This spelling matches the design, but could it be JavaScript instead?

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 12, 2018

When Nodes Have No Attribute

Hi @johnwatkins0,
This looks really good.

This probably isn't a common case. But maybe if an invalid_element doesn't have node_attributes to display, this shouldn't display a <details> element:

details-no-attributes

To reproduce this, you could create a post with a 'Custom HTML' block with:

<script>doThis();</script>
@johnwatkins0

This comment has been minimized.

Contributor

johnwatkins0 commented Sep 12, 2018

Thanks, @kienstra!. I made those changes in my last round of commits.

I'm sending this to code review now. All the AC in #1361 are covered with the exception of the tooltips on the "Status" and "Details" columns (which are potentially part of #1400 now).

Note: in the CSS I make use of icons added in #1397, which hasn't been merged yet. I did add the baseline-check-circle-green.svg icon, however, because I didn't see it anywhere in related open PRs.

Screenshot
errors_by_type_ _local_wordpress_test_ _wordpress

@johnwatkins0 johnwatkins0 requested a review from westonruter Sep 12, 2018

@johnwatkins0 johnwatkins0 changed the title from [WIP] Apply new error taxonomy page design to Apply new error taxonomy page design Sep 12, 2018

@westonruter westonruter self-assigned this Sep 13, 2018

westonruter added some commits Sep 13, 2018

Improve validation error details in list view
* Prevent duplicating parent.
* Use translated heading for attributes.
* Prevent showing details if there are no attributes.
* Prevent PHP notice when validation error lacks type.
@@ -620,32 +635,47 @@ public static function add_admin_hooks() {
add_filter( 'manage_edit-' . self::TAXONOMY_SLUG . '_columns', function( $old_columns ) {
return array(
'cb' => $old_columns['cb'],
'error' => __( 'Error', 'amp' ),
'created_date_gmt' => __( 'Created Date', 'amp' ),
'error' => __( 'Error Inventory', 'amp' ),

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

Actually I don't think this column name makes sense here. If we are going to use “Error Inventory” then this would be for the entire screen, not just for the one column. The column makes more sense as just “Error” I believe. I'll change it.

westonruter added some commits Sep 13, 2018

Add details link to validation error row actions; update column headings
* Restore "Error" as column name instead of "Error Inventory".
* Use just "Type" as opposed to "Error Type".
case AMP_Validation_Error_Taxonomy::VALIDATION_DETAILS_NODE_NAME_QUERY_VAR:
$clauses['orderby'] = $wpdb->prepare(
'ORDER BY SUBSTR(tt.description, LOCATE(%s, tt.description))',

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

@johnwatkins0 This is very clever!

@westonruter westonruter merged commit c287abc into develop Sep 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/1361-taxonomy-page branch Sep 13, 2018

@westonruter westonruter added this to the v1.0 milestone Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment