ampproject/amp-wp

Implement improved URL listing view

postphotos opened this issue · 20 comments

postphotos commented

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 here.
(From Ryan) This is the updated design, right? Final design: [ Page 1 ] [ Page 2 ]

  • AC1: Implement new design elements.

    • New header labels for each column: URL, Status, Invalid, Sources, Last Checked
    • For error "Sources," incorporate the updated design to better organize sources that can be expanded or collapsed. from #1006 (color syntax to separate source of plugin, theme, color) that should consider a11y
    • Surface the validation of whether a given URL is delivering AMP in the index view by conditionally showing the "AMP Logo" (as it's done elsewhere) when errors are suppressed (as shown in the mocks.)
    • Implement new icons and language for each rendered error (which respects Stale states):
      • Suppressed (formerly "Accepted"
      • Identified (formerly "New"
      • To Fix Later (formerly "Rejected"
      • Counts should appear next to each error, as the plugin already does.
      • "New" errors should reflect auto-santization. If auto is on (in Native or in Paired), the icon should be a flag. If auto is off (in Paired) the icon should be a checkmark and red. See here.
    • Implement new language for each rendered error action:
      • Suppress (formerly "Accept"
      • To Fix Later (formerly "Reject"
    • Implement a tooltip helper for the "Status" column that allows us to describe what error states mean in case a user is stuck.
    • Combine the Found Errors and Found Attributes into a single column as shown in the mocks. Also include CSS errors so that all Invalid errors are displayed in this summary view.
    • Change "Created Date" to more accurate language: "Last Checked"

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

  • AC2: I should be able to view the other index listing (Errors by Type) 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

Proposed/Backlog:

  • AC3: I should be able to filter them by error status and type. (see #1373)~
westonruter commented

See #1361 for challenges involved with the filtering by a new error type (category).

miina commented

@postphotos Is the Source column included on the screen out of scope at this moment?

hellofromtonya commented

@miina The source column is part of this sprint and the v1.0 release. The filter by source dropdown is not.

  • 👍2
miina commented

Another question: in the Found elements and attributes column -- would the elements and attributes be separated somehow to show if an element or attribute was found? If we display the detailed view as on the screen then it's not clear which part of it was incorrect:
screen shot 2018-08-30 at 4 19 47 pm
Also, having detailed information about every element/attribute might make the cell content quite long.

Or is this just a mixed list of elements and attributes without details, for example:
screen shot 2018-08-30 at 4 31 56 pm

cc @postphotos @jwold

Thoughts?

jwold commented

@miina yes it is a source of mixed list of elements and attributes without details, thanks for clarifying!

postphotos commented

Hi @westonruter, in your comment here you called out that we've omitted facets for these screens. I also think we should probably keep these for both this and #1361.

cc: @jwold @jillchu

jwold commented

Just added comments to the two designs:

https://sketch.cloud/s/Vow17/all/page-1/amp-validation-urls
https://sketch.cloud/s/Vow17/all/page-1/amp-validation-errors

Asking for adding facets.

Would need to know exactly what facets we want to add.

postphotos commented

The intent of this screen is to show all errors as grouped by URL to allow a user to either recheck or drilldown to a given URL to understand how to action.

kienstra commented

Latest Design

It looks like this this [ Page 1 ] [ Page 2 ] is the latest design for this issue, and the Sketch URL ending in VoDKr xQRzx is the current source of truth:

https://sketch.cloud/s/VoDKr/
https://sketch.cloud/s/xQRzx/

  • 👍1
kienstra commented

Request To Apply Latest Design

As mentioned above, here's the latest design for this:

latest-design-errors-by-url

And here's the current state of this page, using Miina's PR #1397:

errors-by-url

Please push directly to PR #1397.

This is now ready for development for whoever would like it.

kienstra commented

Request To Develop For This

Hi @jacobschweitzer,
Thanks a lot for helping here. Could you please develop for this issue?

westonruter commented

The column header should be “Invalid” as opposed to “Removed”.

Also, for invalid attributes please wrap in square brackets like [onclick] whereas invalid elements can be bare like script. I think they should also be wrapped in code elements.

  • 👍2
kienstra commented

Moving to 'In Progress'

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

jacobschweitzer commented

Thanks @kienstra , I'm used to working with Jira so this setup is new to me. I'll be sure to update the status of my tickets appropriately going forward.

  • 👍2
postphotos commented

@jacobschweitzer and @kienstra, the AC/mocks have been updated. Note: The current mock says "Removed" but it should say "Invalid." The ACs are correct.

  • 👍1
jacobschweitzer commented

@westonruter This is ready for review. Thanks!

kienstra commented

Request For Testing

Hi @csossi,
Could you please test the Invalid URLs page, using these designs in the description for reference? I'll be around if you have any questions.

That site has the develop branch of the plugin deployed (from 2 days ago), up to ec4642d.

Thanks, Claudio!

csossi commented

QA comments:

  • should the "?" for "Status" have a tooltip popup?
  • design has "paired AMP mode with auto-sanitization" in bold
  • design has filters in differnt order (bulk actions, apply, etc.)
  • all statuses here at this URL are "New" - should they be "New Accepted"?
kienstra commented

Thanks For Testing

Hi @csossi,
Sorry for the delay in replying.

should the "?" for "Status" have a tooltip popup?
This probably didn't appear at the time of testing. But now that Beta4 is deployed, it appears:

invalid-urls

design has "paired AMP mode with auto-sanitization" in bold

Good point.

design has filters in different order (bulk actions, apply, etc.)

Also a good point. I'll raise this and the one above with @jwold and Jill when we talk about how well this aligns with the design.

all statuses here at this URL are "New" - should they be "New Accepted"?

These probably didn't display when you tested this. But with #1429 merged, the 'New Accepted' error status appears:

new-accepted-error

kienstra commented

Moving To 'Ready For Merging'

With the points above addressed, I'm moving this to 'Ready For Merging'

design has "paired AMP mode with auto-sanitization" in bold

Based on a design discussion, this is alright.

design has filters in different order (bulk actions, apply, etc.)

The 'All Dates' filter is move to the right of the status and type filters in #1462