Table style update

For no reason I had so much time on trying to create the patch, oh - took more than making the changes themselves.

Includes 4 images for table sorting and removing images for sorting from template.

Fixed all above.
The font is different, that's why it looks like that :)

I didn't touch the font family in this patch. I think it's work for another patch.

+++ b/core/themes/seven/style.cssundefined
@@ -458,30 +467,67 @@ tr.drag {
+  right: 0; /* LTR */

If you're changing these kinda things you need to be sure it is rendered correctly in RTL mode. You can do this by switcing from LTR to RTL in the language settings. Its possible that you need to enable an aditional language to get the settings page.

BTW: please provide screenshots.

@aspilicious thx!
I've did that and found one small bug.

But now I need more information - how a table should look with RTL language?
Attaching screenshot how it looks like right now. (with a small fix for the bug above)
I doubt that rtl table doesn't need any changes.

Why are the table headers red?

@Bojhan:
- looks like maybe there's something rendering as a list-item in there, creating a bullet and some extra space.
- Why do you say those two columns should be right aligned? Is this an RTL display? If so, all the items should be right-aligned.

#9, thead is mistakenly #faf5f2 rather than #f5f5f2

Optimized PNG files! YAY! :)

(Note: I'm no CSS expert, so this is a best-effort attempt at feedback.)

+++ b/core/themes/seven/style.cssundefined
@@ -430,27 +430,36 @@ div.submitted {
+  font-size: .875rem;
+  line-height: 1.125rem;

This only works in IE>=9, which is acceptable now that we don't support IE8 anymore :)

+++ b/core/themes/seven/style.cssundefined
@@ -430,27 +430,36 @@ div.submitted {
+  min-height: 39px;

Why is this in px if the font size is defined in rem?

+++ b/core/themes/seven/style.cssundefined
@@ -430,27 +430,36 @@ div.submitted {
+tbody tr:hover {
+  background: #f7fcff;

I thought this might be problematic on touch devices (like it was for contextual links) but it is not. Yay :)

+++ b/core/themes/seven/style.cssundefined
@@ -458,30 +467,67 @@ tr.drag {
+th > a:after {
+  content: '';
+  display: block;
+  position: absolute;
+  top: 0;
+  bottom: -1px;
+  left: 0;
+  right: 0;
+  border-bottom: 2px solid transparent;
+  transition: all 0.1s;

What's this for? Why the transition?

A bit of docs would be helpful here.

+++ b/core/themes/seven/style.cssundefined
@@ -458,30 +467,67 @@ tr.drag {
+  background-color:transparent;

Nitpick: missing space after colon.

+++ b/core/themes/seven/style.cssundefined
@@ -458,30 +467,67 @@ tr.drag {
+[aria-sort="ascending"].active > a {
+background-image: url(images/arrow-asc-active.png);
+}
+[aria-sort="ascending"].active > a:hover {
+background-image: url(images/arrow-asc.png);
+}
+[aria-sort="descending"].active > a {
+background-image: url(images/arrow-desc-active.png);
+}
+[aria-sort="descending"].active > a:hover {
+background-image: url(images/arrow-desc.png);

These selectors appear to be a tad too broad? http://www.w3.org/TR/wai-aria/states_and_properties#aria-sort says this should be used for tables *and grids*. Though I guess this is sufficient for the 99%? If so, ignore this.

d.o WTF

meta issue added

Hopefully I'm doing it correctly...

Interdiff:
https://dl.dropboxusercontent.com/u/1539612/1986400-4-interdiff.txt

Some notes:

• Colors wise, I followed the Drupal color and selection color as stated in https://groups.drupal.org/node/283223#Color
• I initially wanted to make the arrows as a sprite but couldn't get it to work so fall back to 4 images :(

Screenshots:

I'm assuming status should be NR since #16 submitted a patch.

Woops, I guess it always happens for the first patch :P Will check on why failed and submit another patch.
But I think some help needed, the review failed didn't mention much details...anyone have some idea?

Thanks!

Ok I realize why, missing 2 new images for the hover arrows...fingers crossed!

Interdiff here: https://dl.dropboxusercontent.com/u/1539612/1986400-tables-styling-5-int...

#20 is not ok, it's a 1MB patch, I assume patched from the wrong location.

I've started from scratch, implementing the table as in seventy-eight, including new arrow images using seventy-eight colors.

Notes:
* The aria-sort is not currently implemented, and this needs help.
* Yes, text is larger since the design's font is not in use.
* I left the bullets in for clarity with multiple roles. If any line wrap they would be unintelligible.
* seventy-eight's padding on th and td differs. Unsure if intentional, I equalized them since unusual that checkboxes don't line up, and just looked like a mistake.

Thanks @Bojhan!

* The dropbutton has a space under it (pushing it up from vertically centered), that will need to be corrected in the dropbutton code.

* I can't reproduce 2 arrows, can you tell me or show where? I see "Updated" at admin/content with one arrow. Browser cache?

* Next patch, I will not change font size and it will look proportionately sized.

* The original patch began with the aria-sort attribute selectors, but they never worked. Was there a discussion that this would be the method for toggling the sort arrows? I do not know how to implement this.

I applied patch #21 on a fresh Drupal 8 installation from 'git'.

It strikes me that the arrows do not 'flip' when changing from 'ascending' to 'descending' sorting. The arrow always points downward. ▼

Lastly, I do not see the nice underline under the active table-header.

I've edited the CSS file to address issue with the underlined table-header.

I realize why my original patch was 1mb instead of few kbs...so I did a pull again from the 8.x branch and create the patch again.
Understand this and echoz version is different, but I agree the aria-sort may not work again in this as well.

@ideogram_nl, #21 and #23 explain that the arrows don't flip. Also, the underline css is there and works, shown in the screenshot.

@ironkiat, what is your patch correcting over #21? It is up for review, you're welcome to review it :-)

#27 deviates from seventy-eight and in part is why I had started clean in #21.

This is #21 patch with only the original font size unchanged. This is mainly to keep this patch as the one already under review.

#30: table-style-1986400-30.patch queued for re-testing.

#30: table-style-1986400-30.patch queued for re-testing.

I propose using the current core method providing inline images for table sort indicators, where we get the sort arrow direction toggle but no hover arrow color change, rather than deleting seven's theme_tablesort_indicator override and using background images, where we get the hover arrow color change but no sort arrow direction toggle (#30 patch).

Also this version does not display arrows on hover of non active headers, just the underline and hover color. Seventy-Eight has arrows on every hover, but the previous sortable tables do not, which I believe makes more sense, keeping the arrows only about the sort.

The active arrows are still included in case we invent how to use them.

Removing a theme function is another conversation entirely. Let's try and keep the scope if this issue down.

Yep, #35 patch is keeping it. There's a lot of existing code for tablesort, and this patch is the first that takes advantage of it and has toggling sort arrows.

Correction on arrow placement for previously untested RTL

IMO this can go in. The sort indicator arrows toggle correctly. They do not have a color change on hover. #35 and #21 note other variations from Seventy-Eight and the reasoning.

Re-roll since #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute got committed.

This patch causes some problems on tables that don't have links to the header. Ideally we would have a conditional classes or some kind of mark up to distinguish this situation and add some padding to the s themselves.

I moved the th anchor's top and bottom padding to the th. The th links function the same.

Patch applies well and everything seems to work well.

Nice one echoz!

I've added the tag "styleguide-independent" which means this issue can be committed on it's own without causing regressions.

Here are a few more screens of various tables:

I forgot to post a field UI table

@Bojhan I can't reproduce what you report in #47. I still get the same as we see in #43, #45, #46. Browser cache? Can you provide a screenshot?

I'm setting this back to RTBC since I can't reproduce #47 and I see a new tag "RTBC July 1" and concerned this isn't supposed to miss out.

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

re-roll since #2030925: quote rtl attribute selector landed

The fixes here are CSS and images only, which are not subject to API freeze, so removing the "RTBC July 1" tag since it is irrelevant.

A UI patch with only 52 comments to RTBC? I've been away too long…

Anyway, problem on Firefox 22, see below. I'm pretty surprised actually. I knew position-relativing a cell doesn't work very well but I would have expected IE to complain. The patch even works on IE8…

Love the new style btw :)

The border is below the table instead of at the bottom of the header cell. Look for the arrow indicating the sorting order too, it's above the table on the right instead of inside the header cell.

@nod_ right about Gecko not understanding position: relative; on table cells! I'm am working on this, and looks like I have a solution.

Fix with position: relative; working on anchor tag.

Safari

Firefox

#59: table-style-1986400-59.patch queued for re-testing.

works, thanks :)

Manually tested and looks good... whilst testing found an unrelated bug #2048895: Dblog CSS results in background image repeating

Committed 7272a71 and pushed to 8.x. Thanks!

Added ry5n to the commit message as the designer.

Great, great improvement! Here are two bugs that have cropped up due to this though in the language tables (interface translation itself and content translation configuration): #2049387: Locale string change styles applied too excessively. Let's fix those!

Sorry to come so late to the party with this, but is everyone satisfied with the table row hover background color? It's so light I didn't even see it at first, and I have a nice monitor and good vision. Any chance we could darken it a bit?

Remaining tasks

Review accessibility

I doubt this had any accessibility testing

You removed zebra striping in favor of that hover.
Also, you tag the issue 'needs accessibility review' and it doesn't get committed until that's done.

That's how the rest of core works.

Sorry, I was typing on my phone. Was being brief, didn't mean to be hostile.

I'm not 100% sure that zebra striping is an accessibility improvement by itself, but like @TravisCarden I'm having a harder time than usual seeing it.

It does seem a bit faint, not that the hover is an essential accessibility feature. We could tweak it slightly, as long as we update the global colour scheme.

@bojhan definitely not, I think the hover is a more elegant solution, I just think we need to tweak the shade of blue to be a bit more visible, maybe even a slight transition will help draw the eye.

The blue of hover is noticeably fine for me, although I have quality displays. If it were darker it might be fine, but if we loose the delicacy of that lightness, a darker blue could get ugly.

@LewisNyman, I thought a transition would be a great idea, but it actuallty makes it less noticeable because it's a subtler change, where without a transition, the instant pop of the change is most noticeable.

@Bojhan of course that makes sense, and is why I wrote I assumed it was a good idea. I did try it (began to create a patch) but on a a background color fading in, I thought it was less noticeable, but this is subjective and I'm certainly ok with deferring to designers :-) I just didn't want it to end up less noticeable and because of that made a darker blue that wasn't as delicate.

I love animated transitions. That being said, this patch no longer applies.

The last patch was committed in #63. This got set to needs review because it needs accessibility review.

It looks good to me. Thanks for the clarification @echoz

Should it now be set to fixed?

yes :-)
Thanks @mgifford