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 URL listing view #1362

Closed
1 of 19 tasks
postphotos opened this issue Aug 27, 2018 · 20 comments
Closed
1 of 19 tasks

Implement improved URL listing view #1362

postphotos opened this issue Aug 27, 2018 · 20 comments
Assignees
Labels
Milestone

Comments

@postphotos
Copy link
Contributor

postphotos commented Aug 27, 2018

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 [Compatibility Tool] Improve experience for user based on AMP compatibility #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:

@postphotos postphotos added this to the v1.0 milestone Aug 27, 2018
@westonruter
Copy link
Member

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

@miina
Copy link
Contributor

miina commented Aug 30, 2018

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

@miina miina self-assigned this Aug 30, 2018
@hellofromtonya
Copy link
Contributor

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

@miina
Copy link
Contributor

miina commented Aug 30, 2018

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
Copy link
Collaborator

jwold commented Aug 31, 2018

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

@postphotos
Copy link
Contributor Author

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
Copy link
Collaborator

jwold commented Sep 4, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

kienstra commented Sep 6, 2018

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/

@kienstra
Copy link
Contributor

kienstra commented Sep 6, 2018

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
Copy link
Contributor

kienstra commented Sep 6, 2018

Request To Develop For This

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

@westonruter
Copy link
Member

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.

@kienstra
Copy link
Contributor

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
Copy link
Contributor

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.

@postphotos
Copy link
Contributor Author

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

@jacobschweitzer jacobschweitzer removed their assignment Sep 11, 2018
@jacobschweitzer
Copy link
Contributor

@westonruter This is ready for review. Thanks!

@kienstra
Copy link
Contributor

kienstra commented Sep 18, 2018

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
Copy link

csossi commented Sep 18, 2018

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
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants