Skip to content
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

Implement improved error listing view #1361

Closed
postphotos opened this Issue Aug 27, 2018 · 25 comments

Comments

7 participants
@postphotos
Copy link
Contributor

postphotos commented Aug 27, 2018

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)

@postphotos postphotos added the release label Aug 27, 2018

@postphotos postphotos added this to the v1.0 milestone Aug 27, 2018

@hellofromtonya hellofromtonya added this to Definition in v1.0 Aug 27, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Aug 28, 2018

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.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Aug 28, 2018

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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Aug 29, 2018

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

@postphotos postphotos moved this from Definition to To do in v1.0 Aug 30, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Aug 30, 2018

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

This comment has been minimized.

Copy link
Collaborator

miina commented Aug 30, 2018

@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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 3, 2018

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

@postphotos

This comment has been minimized.

Copy link
Contributor Author

postphotos commented Sep 4, 2018

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.

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 4, 2018

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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 5, 2018

Latest Design

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

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 6, 2018

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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 6, 2018

Feel free to push directly to PR #1394.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Sep 6, 2018

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.

@johnwatkins0

This comment has been minimized.

Copy link
Contributor

johnwatkins0 commented Sep 9, 2018

@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

This comment has been minimized.

Copy link
Member

westonruter commented Sep 9, 2018

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

@johnwatkins0

This comment has been minimized.

Copy link
Contributor

johnwatkins0 commented Sep 10, 2018

@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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 10, 2018

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.

@kienstra kienstra moved this from To do to In progress in v1.0 Sep 10, 2018

@postphotos

This comment has been minimized.

Copy link
Contributor Author

postphotos commented Sep 11, 2018

@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!
@johnwatkins0

This comment has been minimized.

Copy link
Contributor

johnwatkins0 commented Sep 12, 2018

@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

This comment has been minimized.

Copy link
Contributor

johnwatkins0 commented Sep 12, 2018

@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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 12, 2018

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 johnwatkins0 moved this from In progress to Ready for review in v1.0 Sep 12, 2018

@johnwatkins0

This comment has been minimized.

Copy link
Contributor

johnwatkins0 commented Sep 12, 2018

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

@hellofromtonya hellofromtonya moved this from Ready for review to Ready for QA in v1.0 Sep 14, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 16, 2018

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

This comment has been minimized.

Copy link
Collaborator

csossi commented Sep 19, 2018

verified in QA

@csossi csossi moved this from Ready for QA to Ready for Merging in v1.0 Sep 19, 2018

@csossi csossi removed their assignment Sep 19, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.