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

Style error tables and implement tooltips #1245

Merged
merged 24 commits into from Jan 26, 2017

Conversation

Projects
None yet
4 participants
@hbillings
Member

hbillings commented Jan 13, 2017

Implements @ericronne's designs for displaying errors in tables.

Tooltips are done entirely with CSS; no JS fallback required.

Mockup:
screen shot 2017-01-13 at 3 45 36 pm

Table:
screen shot 2017-01-13 at 3 33 55 pm

Tooltip:
screen shot 2017-01-13 at 3 34 02 pm

To do:

  • Fix the tooltip's arrow on non-right-aligned cells (see screenshot)

screen shot 2017-01-13 at 3 44 05 pm

- [x] Fix arrow falling off the tooltip when there's more than one line in the error description.

screen shot 2017-01-13 at 4 31 54 pm

@hbillings hbillings changed the title from Style error tables to Style error tables and implement tooltips Jan 13, 2017

@hbillings

This comment has been minimized.

Member

hbillings commented Jan 13, 2017

Aaargh, multiline cells break >_<
screen shot 2017-01-13 at 4 31 54 pm

Also, @ericronne, if an error spans multiple lines, how should it look? Currently looks like this:
screen shot 2017-01-13 at 4 32 31 pm

@ericronne

This comment has been minimized.

Contributor

ericronne commented Jan 15, 2017

Might help if we shrunk them down to 13px (same size as the status icons in #980). Here are the svg files for the filled info icons and the outlined (hover) version at that size.

@hbillings

This comment has been minimized.

Member

hbillings commented Jan 18, 2017

Okay, this is how it looks now:
screen shot 2017-01-17 at 10 31 13 pm

There's one oddity: Because of how I've chosen to do the tooltip pointer-speech-bubble-arrow-thing, the pointer is different sizes on a cell that is one line deep vs. a cell that is two lines deep. I could use some JS to normalize it, but this is about as good as I can get it with just CSS.

@hbillings

This comment has been minimized.

Member

hbillings commented Jan 18, 2017

So there was this little funky interaction that was bugging me: when you moved the mouse between the info badge and the text, the tooltip would flicker. So I rewrote the HTML, and it looks/works great except I'm not sure multiline cells look as nice now:
screen shot 2017-01-18 at 3 46 56 pm

Thoughts?

@ericronne

This comment has been minimized.

Contributor

ericronne commented Jan 18, 2017

Can we pair on this briefly?

@hbillings

This comment has been minimized.

Member

hbillings commented Jan 19, 2017

After screensharing with @ericronne, we've decided to:

  • normalize the tooltip arrow heights with a little javascript
  • return to underlining all of the text in the error cell instead of just the div it's in...somehow...
  • tighten up spacing between text and underline and text and icon
  • extend dotted line under icon
@hbillings

This comment has been minimized.

Member

hbillings commented Jan 19, 2017

@ericronne whatcha think:
screen shot 2017-01-18 at 6 41 01 pm

Do those arrows look the same to you? I'm going cross-eyed, but I think I found a CSS formula that works for positioning them, no JS needed. Maybe.

@toolness

This comment has been minimized.

Contributor

toolness commented Jan 19, 2017

Hmm, is this implemented for the fake schedule, or just for schedule 70? When doing it on the fake schedule, I'm getting decapitated tooltips on hover, it's very unsettling.

2017-01-19_7-22-08

FWIW, I think this could be a great place to embed an example in our fancy STYLE GUIDE. This would make it possible for us to manually test the styling / interaction without needing to go through the PL upload flow and create our own price list that has errors in it.

I guess one option might be to reuse code in such a way that the styleguide example isn't a duplication of the markup used in the PL upload (which could then easily fall out-of-sync). Let me know if you want me to work on this!

@ericronne

This comment has been minimized.

Contributor

ericronne commented Jan 19, 2017

My only thoughts are A: way better! and B: if it's easy to bring the tip box closer to the text, that would be ideal. Not a deal breaker, but would look better if the boxes were slightly overlapping field edges, rather than aligning with them, like this …
image

@ericronne

This comment has been minimized.

Contributor

ericronne commented Jan 19, 2017

BTW it's probably fine if the tip obscures the top line of two-line content, since this is just a hover state on desktop.

PS which do you prefer: pink bg or red, like so? …

image

@hbillings

This comment has been minimized.

Member

hbillings commented Jan 19, 2017

@toolness Right now it's just for S70, mostly because I'm too lazy to make changes to both sets of markup until I know which is the winning strategy. 😄 (I totally should have thought of the styleguide option for testing, tho, 'cause that would have been so much easier. Womp womp. I'll throw an example in there once the style gets a thumbs-up!)

@ericronne Lemme see what I can do about the positioning! The tooltip's bottom edge is currently positioned to hit 100% of the table cell width, so I'm not sure how to subtract a pixel from that with CSS. I could try setting the bottom edge to be a line-height of the text in the table cell, though...

Re: background, I'm personally not the biggest fan of reverse type, but I'm also not the visual designer on this. 😉

@hbillings

This comment has been minimized.

Member

hbillings commented Jan 19, 2017

@ericronne Okay ignore everything I said before and look at this:
screen shot 2017-01-19 at 10 03 44 am

@hbillings

This comment has been minimized.

Member

hbillings commented Jan 19, 2017

@toolness Circling back around on this, it looks like all the logic is there to render the s70 table automatically in the styleguide, but I can't figure out how to call the to_error_table child method in the styleguide views. Maybe we could pair?

@toolness

This comment has been minimized.

Contributor

toolness commented Jan 20, 2017

Sure! Sorry I couldn't make it to coworking yesterday, but I can definitely pair on it.

@hbillings

This comment has been minimized.

Member

hbillings commented Jan 23, 2017

I can't figure out how to generate the tables for the styleguide automatically, and I need to move on to other things, so I hardcoded it for now. It looks like this:
screen shot 2017-01-23 at 12 37 01 pm

@jseppi

Looks pretty good. The tooltips work great with a mouse, and the look of the errors tables is 👍

I added a couple questions to be addressed. My most concerning one is the one about the column rendering order.

{% endif %}
{% for col in row %}
<td class="{% if col.errors %}error{% endif %} {% if col.name == 'min_years_experience' or col.name == 'price_including_iff' %}number{% endif %}">
<span {% if col.errors %}aria-describedby="error__row-{{ row_count}}__col-{{ forloop.counter }}"{% endif %}>{% if col.name == 'price_including_iff' %}${% endif %}{{ col.value }}</span>

This comment has been minimized.

@jseppi

jseppi Jan 23, 2017

Contributor

Should these spans maybe have tabindex="0" so that they can be tabbed to by keyboard users? Also, I think if that is done, then the tooltips will need to also show on :focus.

This comment has been minimized.

@jseppi

jseppi Jan 23, 2017

Contributor

(I honestly don't know what the best practice is for this stuff, but I just tried that locally and it seemed to work ok)

This comment has been minimized.

@hbillings

hbillings Jan 23, 2017

Member

@jseppi I'm not sure what best practice is either. The tooltips should be read by screenreaders, but making it work for keyboard users would be good. I think there may be a PR for focusable tooltips from way back when -- I'll see if I can find what I'm thinking about + do a little research.

This comment has been minimized.

@toolness

toolness Jan 23, 2017

Contributor

Yeah these should def be focusable by the keyboard-only folks! Hmm.

I think we may have to be careful to put the tabindex="0" on something that makes some sense from an ARIA standpoint... Otherwise if someone using a screen reader tabs to the tooltip, I'm not sure what kind of role it'd announce the currently focused element to have... Is it possible to put the tabindex="0" on the same elements that have role="tooltip"? Not sure if that'd fix things but it might.

This comment has been minimized.

@hbillings

hbillings Jan 24, 2017

Member

So adding tabindex="0" to the table cell works beautifully, but I wanted to do some more research on the accessibility... It looks from this article like tables should have grid and gridcell roles when they're used to display tabular data. (Though that article also notes that aria-readonly should be true.) @atul, does this check out to you? If so, I think I'll leave the tabindex on the td and just fill in the appropriate roles.

This comment has been minimized.

@toolness

toolness Jan 24, 2017

Contributor

Cool! I don't know enough about stuff to know if that's a good fix for sure or not but it seems reasonable. We could always ask bristow too. As long as the error info is at least announced by screen-readers, and as long as the tooltips are keyboard-accessible, though, it seems like a great initial implementation that we can then update later if needed... no need to hold off on merging until the perfect a11y sol'n is found, imo.

This comment has been minimized.

@jseppi

jseppi Jan 24, 2017

Contributor

I think that article is just talking about correcting bad/non-semantic HTML tables with ARIA roles. I'm having a hard time parsing the article, but this line seems salient: "As always the best advice is use native HTML elements whenever possible" (which we do). And their example is correcting a case where there are two tables defined successively, with the first table is used only for headers and the second table is used for content (who would do that?).

Our tables look semantically correct to me so I don't think you need to sprinkle roles all over.

This comment has been minimized.

@toolness

toolness Jan 24, 2017

Contributor

Yeah, our tables are semantically correct for sure--my concern was just that screenreader users might be confused as to why some of the parts of the table, which are non-interactive to them, are focusable in the tab order. Normally only interactive things should be focusable in the tab order--and while that's true for sighted keyboard users, it's not true for screenreader users, because the tooltip content is showing by default to them (no interactivity required), at least that's my understanding.

But still, even if I am right, it's probably not enough of an issue to block this PR on it--as long as the content is still somehow accessible to screen reader and keyboard users, I think that's great for a first implementation, even if it might be a bit confusing or awkward.

{% else %}
{{ row.price_including_iff.value|floatformat:2 }}
{% endif %}
{% for col in row %}

This comment has been minimized.

@jseppi

jseppi Jan 23, 2017

Contributor

hrm, I'd be a little wary of looping the columns here but not doing the same for the headers. I could imagine the hardcoded headers getting "out of sync" with the order of the columns. Do we have any guarantee anywhere that the columns will always render in the same order as the hardcoded headers?

This comment has been minimized.

@jseppi

jseppi Jan 23, 2017

Contributor

Aha, I was having trouble figuring out where the order came from at all. Turns out the fields get iterated in the order they are defined (like in Schedule70Row for instance). So, that means this loop is pretty brittle. If another field is added or if just the formatting of the Schedule70Row class is changed, then the table will get wrecked.

For instance:

Slightly modifying Schedule70Row to put unit_of_issue at the top:

class Schedule70Row(forms.Form):
    unit_of_issue = forms.CharField(
        label="Unit of issue",
        validators=[hourly_rates_only_validator]
    )
    sin = forms.CharField(label="SIN(s) proposed")
    labor_category = forms.CharField(
        label="Service proposed (e.g. job title/task)"
    )
    education_level = forms.CharField(
        label="Minimum education / certification level",
    )
    min_years_experience = forms.IntegerField(
        label="Minimum years of experience"
    )
    price_including_iff = forms.DecimalField(
        label="Price offered to GSA (including IFF)",
        validators=[min_price_validator]
    )

yields this borked table:

screen shot 2017-01-23 at 3 14 38 pm

This comment has been minimized.

@hbillings

hbillings Jan 23, 2017

Member

Eeeek! Would looping the headers as well solve this, or should I revert the tables to the original verbose output?

This comment has been minimized.

@toolness

toolness Jan 23, 2017

Contributor

Hey, there's a new feature in Python 3.6 (see #1265) that might maybe help with this, I'm not sure. It's called PEP 520 -- Preserving Class Attribute Definition Order. I think before 3.6 there might not have been an actual guarantee that the fields got iterated in the order they were defined, but now there will be, or something. 🤷‍♂️

This comment has been minimized.

@jseppi

jseppi Jan 24, 2017

Contributor

I would say I am more for being explicit than looping the fields. I wonder though if there is a way you could do something like

table_fields = ['sin', 'labor_category', ... , 'price_including_iff']

and then loop through those in the template (non-working pseudo-code):

{% for field in table_fields %}
  <td>{{ row[field] }}</td>
{% endfor %}

@toolness?

This comment has been minimized.

@toolness

toolness Jan 24, 2017

Contributor

Yeah I think that's definitely a way we could do it... passing table_fields in as a context variable, I assume?

This comment has been minimized.

@jseppi

jseppi Jan 24, 2017

Contributor

Or maybe defining it as an attribute in the SubmittedPriceListRow model

This comment has been minimized.

@toolness

toolness Jan 24, 2017

Contributor

Hmm, imma hack on this for a bit methinks. Be back soon!

This comment has been minimized.

@toolness

toolness Jan 24, 2017

Contributor

Ok I attempted to hack up a solution in #1268. Basically we just pass an empty Schedule70Row instance to the template and iterate through its form fields, much in a similar way that we'd do to render an actual form, only in this case we're using the form field metadata to render the <th> tags instead of form fields.

@ericronne

This comment has been minimized.

Contributor

ericronne commented Jan 24, 2017

Once this is all sussed, y'all totally need to give a talk on it.
Eg here. Kinda far, tho :/

hbillings and others added some commits Jan 24, 2017

Merge pull request #1267 from 18F/dry-styleguide-error-table
Make styleguide reuse s70 error table template.
@jseppi

jseppi approved these changes Jan 24, 2017

@hbillings hbillings merged commit cf52903 into develop Jan 26, 2017

3 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hbillings hbillings deleted the error-tables branch Jan 26, 2017

@jseppi jseppi referenced this pull request Jan 31, 2017

Merged

Tag and release v2.1.0 #1303

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