ampproject/amp-wp

Implement improved error listing view

postphotos opened this issue · 25 comments

postphotos commented

As an developer using the AMP for WordPress plugin when viewing AMP errors by type, I seek improved usability though color highlighting, improved wording and new visual styles.

Efforts to implement the dropdown facet filters should be discussed here: #1363

Based on "errors by type" screen from here here._
(From Ryan) This is the updated design, right? FINAL DESIGN: [ Page 1 ] [ Page 2 ]

As an developer using the AMP for WordPress plugin when viewing AMP errors by URL, I seek improved usability though color highlighting, improved wording and new visual styles.

Based on "Errors by URL" screens from here.

  • AC1: Implement new design elements.

    • New header labels for each column: Error Inventory, Status, Details, Error Type, Last Seen, Details, Last Seen and Found URLs
    • Implement a type column using the new language here, referencing the mocks and work IP elsewhere (see #1360)
    • Implement new icons and language for each rendered error (which respects Stale states):
      • Suppressed (formerly "Accepted"
      • Identified (formerly "New"
      • To Fix Later (formerly "Rejected"
      • No counts should appear next to errors, as the plugin already does.
    • Implement new language for each rendered error action:
      • Suppressed (formerly "Accepted"
      • Identified (formerly "New"
      • To Fix Later (formerly "Rejected"
    • Implement a tooltip helper for the "Status" column that allows us to describe what error states mean in case a user is stuck.
    • Change "Created Date" to more accurate language: "Last Seen"

  • I should also still be able to take bulk actions on these errors.

  • Search, error count and pagination (regular feature of these index listings of errors) should remain.

  • For details, errors should be expandable and collapsible to make the interface as easy as possible to navigate.

  • Error Inventory, Details, Error Type should all be sortable from A-Z. Details should be sortable by node name.

  • Last Seen should be sortable as usual time elements are.

  • Found URLs should be sortable by count.

  • AC2: I should be able to view the other index listing (Errors by URL) screen by clicking on the button next to the page title in this listing. The wording of the page title and sidebar for this feature should match the mocks.

Efforts to implement the backlog item of dropdown facet filters should be discussed here: #1363

- [x] AC3: I should be able to filter them by error status and type.(see #1373)

westonruter commented

What are the types?

  • CSS
  • HTML (Element)
  • HTML (Attribute)
  • JavaScript

Note that organizing validation errors by type will be tricky because the validation_error is already a taxonomy. We are implementing the validation error status by using the infrequently-used term_group field, but there is only one such field to use here. Since the field is an integer it would seem at first feasible to use that integer to store bit flags and then use a bit mask to do the search. However, it does not appear that MySQL indexes fields for bitwise operations, so this would require doing an IN enumeration for all possible values.

For example, given an 8-bit integer, where the first two digits are dedicated for the status:

  • 00 = new
  • 01 = accepted
  • 10 = rejected

The remaining 6 bits could potentially be used to store the type information. For example:

  • 01 = CSS error
  • 10 = HTML error
  • 11 = JS error

So then those could be combined as follows:

  • 0001 = new CSS error
  • 0110 = accepted HTML error
  • 1011 = rejected JS error
  • …and 6 other possible combinations.

So if you wanted to get all CSS errors regardless of the type, then this would require a condition like:

term_group IN ( b'0001', b'0101', b'1001' )

Something like this would have to replace:

https://github.com/Automattic/amp-wp/blob/14d6e4b6570c9b57ef73f704af633aec6ed04b70/includes/validation/class-amp-validation-error-taxonomy.php#L390

And then doing an update of a term_group would require bitwise math to update the bits at:

https://github.com/Automattic/amp-wp/blob/14d6e4b6570c9b57ef73f704af633aec6ed04b70/includes/validation/class-amp-validation-error-taxonomy.php#L992

All of this to say that it is possible, but it is complicated.

  • 👍1
westonruter commented

UPDATE: See #1360 (comment) for an update on a solution for this which relies on doing fulltext searches of the JSON blob in the wp_term_taxonomy.description field.

kienstra commented

Question About Bulk Actions

Hi @postphotos,
Thanks for this issue.

I should also be able to take bulk actions on these errors.

The existing bulk actions are 'Accept' and 'Reject.' Are there more bulk actions that you'd like?

existing-bulk-actions

westonruter commented

The existing bulk actions are 'Accept' and 'Reject.' Are there more bulk actions that you'd like?

No. There aren't any other possible actions to take, AFAIK.

miina commented

@postphotos The labels for each column and the new "Type" column don't seem to exist on the "Errors by URL" screen mentioned in the description of the ticket. Did you mean "Errors by Type" (screen)?

kienstra commented

WIP PR #1394 applies some of the changes here, but I'm not sure how finalized the design is.

postphotos commented

The intent of this screen is to show all errors as grouped by type (HTML, CSS, JS) and to allow a user to either action or drilldown to a given error.

  • 👍1
kienstra commented

Question About 'Details' Column

Hi @postphotos,
Could you please help with a question about the 'Details' column in the design?

body-aside-footer

In that column, what do 'Body,' 'Aside,' and 'Footer' refer to? Are those the elements in which the error occurred?

If so, I don't think there's currently a way to detect which of those the element is inside. There is a parent_name field for elements, but I don't think it's often body or aside.

kienstra commented

Latest Design

As far as I know, this is the latest design for this issue.

kienstra commented

Current State, Remaining Tasks

Hi @jacobschweitzer,

Could you please work on this 'Errors by Type' page, by continuing the work on PR #1394?

Current state, with PR #1394:
current-state-pr-1394

Should match this design:
design

kienstra commented

Some Remaining Tasks

  1. 'Details' column: change the formatting, and apply the new colors
  2. 'Details' column: Use <details> elements, with nested <summary>
  3. Style that nested <summary> so that the 'caret' marker appears on the right, maybe like:
 summary::-webkit-details-marker { float: right; color: blue }

details-caret-right

  1. On clicking the 'double caret' next to the 'Details' header, that should expand all of the <details> elements:

design

  1. 'Error Inventory' column: make all error types link to their single error page

Don't worry about (yet):

  • The 'Status' and 'Details' icons and tooltips
  • The 'All,' 'JavaScript,' 'HTML,' 'CSS' links (those are replaced by a filter)

Most of these changes are likely to be in AMP_Validation_Error_Taxonomy.

Details Column

In the design, I think the 'Body,' 'Aside,' etc... are the parent_name, stored in the error term:

parent-name

kienstra commented

Icons

@miina's PR #1397 for Issue #1362 has icons, including the AMP logo for 'Accepted.' So there's no need to separately implement them here.

kienstra commented

Feel free to push directly to PR #1394.

westonruter commented

Yes, the details column can show the parent_name when they are collapsed.

Also, it should show <body>, <aside>, etc instead of “Body”.

Also, I'm not yet sold on how the node_attributes are presented. So don't focus on implementing that as designed yet.

  • 👍2
johnwatkins0 commented

@kienstra I have a question on

'Error Inventory' column: make all error types link to their single error page

Can you clarify what you mean by the "single error page?" From what I've seen, it appears terms in the error type taxonomy aren't meant to be editable, so we wouldn't want to use a standard edit term link. It might make sense to link to the "invalid_urls" listing with posts for the term (same as the link in the "Found URLs" column). I can't think of what else we might link to there. Let me know what I'm missing. Thank you.

westonruter commented

@johnwatkins0 hey, it's the same link as is currently shown in the Count column. Clicking on the term count link takes you to a view of all amp_invalid_url posts that have that specific amp_validation_error associated with it.

  • 👍2
johnwatkins0 commented

@postphotos Two questions.

1
In the AC you mention "Errors, Status, Type, Last Checked, Details, Last Seen and Found URLs" as the columns. In the mockups we have "Error Inventory, Status, Details, Error Type, Last Seen, Found URLs." Should I follow the mockup or the AC? Also, what's the difference between Last Checked and Last Seen?

2
sketch_cloud_-_amp_validation__errors

In this photo, the Details and Error Types columns are links, indicating the list should be sortable by these two columns. Just wanted to confirm that this is a feature we should be implementing. If the Details column is sortable, are we sorting alphabetically by the displayed element/parent node?

kienstra commented

Moving to 'In Progress'

Hi @johnwatkins0,
If it's alright, I'm moving this to 'In Progress,' to reflect the fact that you're working on it.

  • 👍2
postphotos commented

@johnwatkins0 Hi John! You caught a stale ticket and it's been reflected in updated ACs and updated link to final mock. 👍

  1. Error Inventory, Status, Details, Error Type, Last Seen, Details, Last Seen and Found URLs
    ^ That's what is now in both the AC and mock.

"Last Checked" became "Last Seen."

  1. Yes - I've added one more AC task to the list. Ping me if you have any questions here!
  • 👍1
johnwatkins0 commented

@postphotos Do I understand correctly that

Implement a tooltip helper for the "Status" column that allows us to describe what error states mean in case a user is stuck.

is not required for the RC release?

If so, do you think we should remove that from this AC and make it part of #1400 ?

johnwatkins0 commented

@postphotos @westonruter @kienstra I'd appreciate any guidance you can provide on the expanded details view. This shows what I have currently:

errors_by_type_ _local_wordpress_test_ _wordpress

For invalid attributes, I'm including that "Parent node" line because as a developer I think that's crucial. For other types of errors I'm following the design.

I definitely find myself wishing there were more details about why each error exists. Also, I think including irrelevant attributes in the details might be confusing (e.g., listing the id attribute along with the invalid viewbox attribute).

I don't want to go too far on this on my own (especially since I'm new to the project), so let me know where I should be headed with this for the RC stage at least.

kienstra commented

Looks Good
Response To Questions

Hi @johnwatkins0,
Thanks, this looks good.

For invalid attributes, I'm including that "Parent node" line because as a developer I think that's crucial. For other types of errors I'm following the design.

Good idea, it's really helpful to see the parent node:
invalid-attributes

Also, I think including irrelevant attributes in the details might be confusing (e.g., listing the id attribute along with the invalid viewbox attribute).

Good point. Though I think it looks alright as is, given that the invalid attribute is mentioned twice, and the other attributes only appear on expanding the <details>:
invalid-attribute

johnwatkins0 commented

@westonruter Moving this to Code Review and reassigning to you for PR #1394. Thanks!

kienstra commented

Request For Testing

Hi @csossi,
Could you please test the Error Index page, using the ACs and the designs in this issue's description?

I just deployed the develop branch of the plugin to that site, up to ec4642d.

Thanks, Claudio!

csossi commented

verified in QA