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

On the single error term page, apply a similar UI to the single URL details #1449

Merged
merged 17 commits into from Sep 22, 2018

Conversation

Projects
None yet
4 participants
@kienstra
Collaborator

kienstra commented Sep 21, 2018

@johnwatkins0 had created a nice display of the details, which expands on clicking the 'Error' row on the single URL page:

single-error-page-details-ui

Something similar to this should be applied to the single taxonomy term UI:

current-ui

Begin to apply John's work on details to the single error taxonomy
John had created a nice display of the details,
which expands on clicking the 'Error' row on the single URL page.
This begins to apply it to the single error term page.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 21, 2018

Request To Work On This

Hi @johnwatkins0,
Could you please work on this? Feel free to push to this branch.

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 21, 2018

@kienstra I think this certainly looks better than the red/blue colors:

image

More alignment between the two styles is needed of course. Perhaps it should consistently be fixed font for keys and values?

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 21, 2018

Hi @westonruter,

Perhaps it should consistently be fixed font for keys and values?

Good idea.

Is this what you had in mind?

fixed-font

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 21, 2018

@kienstra Each value should be have a fixed-width font, and the values should be indented as well. No need for the braces or the quote marks. For the nested children, in that case the key can just be bold with the value not-bold. The top-level keys could have the gray background like you show above. Disclaimer: I'm not a designer.

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

@johnwatkins0

This comment has been minimized.

Contributor

johnwatkins0 commented Sep 21, 2018

@westonruter @kienstra Here's how it looks with the commit just pushed:

invalid_urls_ _local_wordpress_test_ _wordpress

@johnwatkins0

This comment has been minimized.

Contributor

johnwatkins0 commented Sep 21, 2018

Based on @westonruter comment a little while ago, I'll change "text : javascript" to "text: javascript".

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 21, 2018

Thanks, @johnwatkins0. This looks really good.

@johnwatkins0

This comment has been minimized.

Contributor

johnwatkins0 commented Sep 21, 2018

@westonruter This is ready for your review.

@johnwatkins0 johnwatkins0 changed the title from [WIP] On the single error term page, apply a similar UI to the single URL details to On the single error term page, apply a similar UI to the single URL details Sep 21, 2018

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

@@ -1903,8 +1903,15 @@ public static function render_single_url_error_details( $validation_error, $term
<div class="detailed">
<?php if ( is_string( $value ) ) : ?>
<?php echo esc_html( $value ); ?>
<?php else : ?>
<pre><?php echo esc_html( wp_json_encode( $value, 128 /* JSON_PRETTY_PRINT */ | 64 /* JSON_UNESCAPED_SLASHES */ ) ); ?></pre>
<?php elseif ( is_array( $value ) ) : ?>

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

This looked to have changed the display of the sources in the single error URL details:

single-error-url

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

Addressed with 5ebc340:

single-err

This comment has been minimized.

@johnwatkins0

johnwatkins0 Sep 21, 2018

Contributor

Thanks for catching that, @kienstra

This comment has been minimized.

@kienstra

kienstra Sep 21, 2018

Collaborator

All good, thanks a lot for applying this UI.

Use previous means of outputting sources, as they don't apply on the …
…single term page

Before, sources were output with wp_json_encode(),
using a pretty-print constant.
Because they don't appear on the single term page,
this commit shouldn't affect that.
But restore the old means of outputting the sources
for the single error URL page.
@amedina

This comment has been minimized.

Member

amedina commented Sep 22, 2018

  • In the single error term page, the top box with the error details should be open by default

screen shot 2018-09-21 at 5 55 23 pm

@amedina

This comment has been minimized.

Member

amedina commented Sep 22, 2018

  • And perhaps the inner components should be open as well; the reason being that the user will go there specifically to look at the details of the errors and the URLs that have it
@westonruter

This comment has been minimized.

Member

westonruter commented Sep 22, 2018

  • For the details on the removed, why are the angle brackets removed? It looks truncated. Also, shouldn't it be monospace?

image

And the node attributes is mostly just a duplicate of this. There should be a vertical list of attributes instead.

  • And all except sources should probably be expanded by default.
@westonruter

This comment has been minimized.

Member

westonruter commented Sep 22, 2018

  • Need to figure out why the sources are being duplicated for each row. I'll fix this.

image

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 22, 2018

  • The order of the errors no longer corresponds to the document order. I'll look into that.

westonruter and others added some commits Sep 22, 2018

@johnwatkins0

This comment has been minimized.

Contributor

johnwatkins0 commented Sep 22, 2018

Just pushed changes for the following:

  • In the single error term page, the top box with the error details should be open by default
  • And perhaps the inner components should be open as well; the reason being that the user will go there specifically to look at the details of the errors and the URLs that have it
  • Also, shouldn't it be monospace?
  • And all except sources should probably be expanded by default.

@westonruter, on "For the details on the removed, why are the angle brackets removed?", that field is the attribute key and values concatenated to look like the removed attributes. I suppose the element could be prepended (with angle brackets) to make it look like an actual element, but I'm not comfortable enough with all the different conditions to make that happen.

westonruter added some commits Sep 22, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 22, 2018

I'm changing the plugin/other sources to eliminate the details when there is only one item.

So going from:

image

To:

image

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 22, 2018

  • The column checkboxes are erroneously including the tooltips:

image

image

westonruter and others added some commits Sep 22, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 22, 2018

on "For the details on the removed, why are the angle brackets removed?", that field is the attribute key and values concatenated to look like the removed attributes. I suppose the element could be prepended (with angle brackets) to make it look like an actual element, but I'm not comfortable enough with all the different conditions to make that happen.

Here's what I'll push that uses a mark element. For an invalid element:

image

For an invalid attribute:

image

image

westonruter and others added some commits Sep 22, 2018

@westonruter westonruter merged commit 42dfafd into develop Sep 22, 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 update/single-error-details branch Sep 22, 2018

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