Problem/Motivation

Tables on several pages, especially admin pages, are too wide for narrow screens. Mobile users have to zoom in to read the columns for administrative tables. This requires horizontal scrolling to read the rest of the row and it becomes difficult to track which row the user is looking at.

Proposed resolution

This patch implements the "Maggie" approach discussed in #2 - #4. We implemented A Responsive Design Approach for Complex, Multicolumn Data Tables, but chose to simplify the UX with a single link to 'Show all columns' instead of a 'Column' selector widget. The Show all columns toggle is smart enough to show only when there are hidden columns.

There is a new, simple API for callers to theme_table(). Each $header can now add an RWD_ADVISABLE (RWD is a common acronym for Responsive Web Design) class or a RWD_HELPFUL class. Both of these are hidden for narrow devices; advisable shows up for tablet widths, and helpful columns show up for desktop widths. Columns with no responsive class are assumed to be essential and always shown. See node.admin.inc and others in this patch for a use examples.

We went back and forth about where to put the CSS and JS for responsive tables. We ended up putting the CSS in the individual themes and the javascript in /misc because the javascript functions independently of the responsiveness breakpoints. This decision may be revisited once we have system-wide breakpoints in core.

Remaining tasks

  1. The 'Show row weights' link should be on the same line as the 'Show all columns' toggle link.
  2. #69:

    ...
    4. the regex thing looks ugly, can't we get rid of the $().hide() it comes from?

    We plan to use drupal-cell-hidden class as per #68.

User interface changes

Columns are hidden for narrow devices. Show all columns link shows columns that are hidden.

API changes

theme_table() now accepts two new classes on header cells and a 'responsive' => FALSE in $variables will disable all responsive handling.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JohnAlbin’s picture

Issue tags: +mobile

Here's an interesting technique for responsive data tables. http://css-tricks.com/9096-responsive-data-tables/

moshe weitzman’s picture

A more modern versio nof that article is at http://css-tricks.com/responsive-data-table-roundup/. Of the techniques listed there, I really think the "Maggie" approach is best suited to Drupal admin. Her article and demo are at http://filamentgroup.com/lab/responsive_design_approach_for_complex_mult.... I'm curious what others think of this approach. It is both usable and doable IMO.

jefflinwood’s picture

I'm working on making the D8 admin responsive, and this issue here is one of the key problems.

I like Maggie's approach as well, but my understanding of it is that every module in D8 (contrib or core) with a table would have to specify which columns get priority. Given that, do you still think this is a doable approach for Drupal 8? I do think it's an elegant solution.

moshe weitzman’s picture

I do think we can expect all core admin tables to use this scheme. Contrib themes which don't set priority are out of spec and persistent misbehavior like this would encourage alternates to arise. So, go Maggie!

I'm not sure if this requires patching theme_table, writing a wrapper for it, or other approach. It would be nice to guide coders along this transition.

jefflinwood’s picture

I think the next step will be for me to add this to the Seven theme, and to update some of the table output in system.admin.inc in the System module to see how it looks. From first glance, I don't think I will need to change theme_table at all, although the documentation might need to change to reflect the new attribute.

I hope for coders, it's as easy as specifying essential, optional, or persistent in the headers they pass into theme_table.

LewisNyman’s picture

Here's a good idea. Let's make every table in core an unordered list instead.

Why are they even tables to begin with? I'd love to know.

yoroy’s picture

Well, every table in core that doesn't have to be a table to present its content :) But yes, slimming down the amount of data in these tables would be a good approach.

Which tables are we talking about here? Would be good to compile a list (ha!).

LewisNyman’s picture

All these need to be converted to the same mark up as this page: http://example.d8mux.org/admin

yoroy’s picture

Thanks!

With themes page we wanted to do some explicit visual weighting. We found people had a hard time discerning default theme from enabled from disabled ones. Also, screenshots are important here.

yoroy’s picture

What about

/admin/content
/admin/content/book
/admin/content/comments
/admin/content/book/*

Lets ignore blocks page for this for now :)

FreekyMage’s picture

So I'm trying to make a complete list (with all modules enabled). This might be overkill, but at least this way we know what we will need to test.

tables with 2 columns or that should be easier to convert:

  • /admin/dashboard
  • /admin/content/book
  • /admin/structure/types
  • /admin/structure/menu
  • /admin/structure/taxonomy
  • /admin/people/permissions/roles
  • /admin/config/people/ip-blocking
  • /admin/config/media/image-styles
  • /admin/config/media/image-styles/edit/large
  • /admin/config/regional/date-time/formats
  • /admin/config/regional/date-time/locale
  • /admin/config/system/actions I heard this is going to disappear though
  • /admin/reports/status

tables with more columns (the ones that end on "fields" and "display" are very similar, but should be tested seperatly anyway):

  • /admin/content
  • /admin/content/comment
  • /admin/content/comment/approval
  • /admin/structure/block
  • /admin/structure/contact
  • /admin/structure/types/manage/page/fields
  • /admin/structure/types/manage/page/display
  • /admin/structure/types/manage/page/comment/fields
  • /admin/structure/types/manage/page/comment/display
  • /admin/structure/forum
  • /admin/structure/menu/manage/main-menu
  • /admin/structure/taxonomy/tags
  • /admin/structure/taxonomy/tags/fields
  • /admin/structure/taxonomy/tags/display
  • /admin/appearance/update & /admin/modules/update , not sure if there is a difference
  • /admin/people
  • /admin/modules
  • /admin/modules/uninstall
  • /admin/config/people/accounts/fields
  • /admin/config/people/accounts/display
  • /admin/config/content/formats
  • /admin/config/search/path
  • /admin/config/regional/date-time
  • /admin/config/regional/language
  • /admin/config/regional/language/detection
  • /admin/config/regional/translate
  • /admin/config/user-interface/shortcut
  • /admin/config/development/testing
  • /admin/config/services/aggregator
  • /admin/reports/dblog
  • /admin/reports/fields
    • These are all kind of similar, maybe some ux expert should look at these to make them more consistent?
    • /admin/reports/hits
    • /admin/reports/access-denied
    • /admin/reports/page-not-found
    • /admin/reports/referrers
    • /admin/reports/search
    • /admin/reports/pages
    • /admin/reports/visitors

Tables that can change the amount of columns:

  • /admin/people/permissions

Not sure if this needs fixing:

  • /admin/config/search/settings
xjm’s picture

Awesome research. Thanks @FreekyMage!

yoroy’s picture

This looks like an issue that could do with some prototype code to see things in action. Ugly, hacky code that gets us quickly to a point where we can evaluate the UX. Seeing it in action will be compelling :)

LewisNyman’s picture

Ask and you shall receive :) Hacky code is my forte:

http://tables.d8mux.org/?q=admin/content

webchick’s picture

Nice. :D My concern though with that example specifically is we lose the checkboxes entirely after the first "shrink point" (sorry I don't know the right term) and the ability to perform bulk operations on this data is one of the primary points of this page.

LewisNyman’s picture

This is true. The way I've designed through this in the mobile prototype is to separate the 'find & navigate' function of a list with the 'bulk edit' function. That way we can minimize interface chrome in till we really need it.

If you hit nav.d8mux.org and navigate down to the node listing page you will notice a button that looks like a bulleted list in the toolbar. This activates bulk edit mode!

I need to do a screencast with all this.

gmclelland’s picture

I started researching it and I found a couple of possible solutions.

  1. http://mobifreaks.com/user-interface/responsive-and-seo-friendly-data-ta... - Gets the styling right(labels bolded and on top), works on Android and IE, requires div markup for tables
  2. http://www.useyourfred.com/2012/01/js-in-my-sexy-responsive-data-tables/ - shows how to do the above with jquery
  3. similar alternative - http://forrst.com/posts/Better_Fluid_Tables-II7

Maybe someone could code up a jquery plugin that would take existing tables and transform them into divs with the proper classes.

http://css-tricks.com/responsive-data-tables/ and http://css-tricks.com/responsive-data-table-roundup/ - note Chris Coyier's technique calls for css to be modified for every table and doesn't work in IE, which wouldn't work.

Hope that helps,
-Glenn

gmclelland’s picture

gmclelland’s picture

I posted a little to quick. After looking at it more, it isn't yet a jquery plugin. It is just a script that looks like it could be turned into a plugin.

The script needs some work. I started http://jsfiddle.net/gmclelland/4EPYW/ this to make some changes to the demo.

So here is a quick little comparison.
http://www.useyourfred.com/2012/01/js-in-my-sexy-responsive-data-tables/
- this requires you to edit the css data attributes for each table.
- not a jquery plugin
- uses existing table markup and changes to divs when loaded
- labels on left, data on right - looks squished on mobile
- Doesn't use html5 data attributes

http://mobifreaks.com/coding/responsive-data-tables-jquery/
- uses html5 data attributes
- labels on top - looks good on mobile
- uses existing table markup and changes to divs when loaded
- not a jquery plugin
- no manual editing of the css data attributes needed per table

Basically I think we need to combine both approaches.
A few questions though: This relies on media queries to work, so how do we know when to collapse a table down to the mobile view?

LewisNyman’s picture

What do people think about a mobile first approach here? Maybe loading in a ul and then converting it into a table for larger screens?

The way most mobile apps handle stuff like this is to have very little information in a list and allow users to drill down to see more information. Your contacts app should be a good example

gmclelland’s picture

The scripts I posted above progressively enhances the tables into divs because tables for some reason won't collapse very well in a cross browser fashion.

I'm not sure changing tables into something else server side would be good here. That may work for Drupal core generated tables, but we also have to think about inline user submitted content/tables.

nod_’s picture

we could dig around using li/divs and display:table-cell to make things look like a table without having to change the makup when switching from one thing to the other.

gmclelland’s picture

@nod_ - thats what the script in #19 does. It replaces table markup with divs. These divs are styled to look like a table. When the browser reaches a certain width the divs collapse down in a linear fashion each table row takes up the full width in a stacked layout.

Try the demo - http://jsfiddle.net/gmclelland/4EPYW/
resize the desktop browser. I can't seem to get jsfiddle to work correctly on a mobile. If you want to test on mobile try http://www.mobifreaks.com/wp-content/demos/Responsive-and-SEO-Friendly-D... It does work on Android.

The author at mobifreaks.com say's he is going to make it an actual plugin, join in on the discussion http://mobifreaks.com/coding/responsive-data-tables-jquery/. If he doesn't, I will try to make it into a reusable plugin.

nod_’s picture

Oh right, didn't read it properly the first time, sorry. I assumed it was doing something else.

Messing with the DOM, especially tables and on mobile is deadly slow, we have typically 50+ row on admin table pages (and way more in tabledrag menus). I'm not convinced this approach will scale. That's a good start though.

gmclelland’s picture

The DOM manipulations are a concern, but I when I fired it up on a my phone(Samsung Galaxy S-Captivate) I didn't see any slow down. If we go the responsive tables route, we will have to optimize the script and figure out how to handle other interactions that happen on tables. For example, sorting table columns and drag and drop(which isn't mobile friendly).

LewisNyman’s picture

What about older, slower phones? It's all very well testing on the latest and greatest. Even my iPhone starts to chug when I'm switching between a lot of apps. We should also be thinking about battery performance.

Here's an example of how it would work if we start with divs and enhance into tables: http://tables.d8mux.org/?q=admin/content

nod_’s picture

Oh wow, this look awesome lewisnyman. Seriously, just wow. And that will make the tableheader script for admin tables way, way easier.

gmclelland’s picture

@lewisnyman - my phone is a older/slower phone. I think they have the Galaxy SIII out now. :)

I think it would be strange for module developers to create their table elements using the fapi but then have drupal change the output server side to divs/uls. I still think manipulating tables client side is the better way to go. Some themes may not want drupal to output their tables into something else.

We also still have the problem of user submitted content that includes tables in the node body. How are we going to make those tables adapt on a mobile?

--------------------
Now that I think about it some more, why should we be converting table elements(tr,td,thead) into divs? Why not simply insert the values of TH from the THEAD at the top of a TR into a TH? This would be more performant as there would be a lot less DOM manipulation.

nod_’s picture

This issue is about Administrative tables, not theme('table') things. It's for editor experience, not for a general catch-all solution, look at the issue title.

There is no DOM manipulation watsoever in the demo, only CSS. and you can actually use CSS3 selectors for hiding stuff because mobile browsers are way smarter than IE8. For me, this is the way to go about admin tables.

(edit) this stuff even work on opera mini (proxy mobile browser).

gmclelland’s picture

@nod_

This issue is about Administrative tables, not theme('table') things. It's for editor experience, not for a general catch-all solution, look at the issue title.

- those tables are created using the fapi correct? Why not cover both admin tables and user submitted content at the same time, both need to be responsive?

There is no DOM manipulation watsoever in the demo, only CSS. and you can actually use CSS3 selectors for hiding stuff because mobile browsers are way smarter than IE8. For me, this is the way to go about admin tables.

- Yes, I know there is no DOM manipulation client side, but there is server side because those tables are generated with the fapi table declarations.

nod_’s picture

It's all happening in theme_tableselect, just override it in seven and that'll work without messing anything, this is not the hard part of this problem (and that's probably how it was implemented in the current demo?)

moshe pointed me out to http://filamentgroup.com/lab/responsive_design_approach_for_complex_mult... which is another solution. I'm not found of it but that exists as well.
(already pointed out to before, don't mind me)

LewisNyman’s picture

I'm not really interested in solving this problem for everyone. There
Are so many options I think it should be left up to the themer. There is no fix all solution here.

The scope of this issue is for the admin interface.

sun’s picture

Note that #991454: Add element #type table (with tableselect and tabledrag support) is still on my table of todos and doing that will dramatically simplify this issue and its implementation, because it entirely removes all of those wonky-donky custom theme functions. Instead, it plugs the entire table + sticky-tableheader + tablesort + tabledrag functionality into a single #type table. Simplification on all ends. Working implementation already exists for D7 in Edge. Will try to resurrect that issue shortly.

err, the above is the Edge issue; this one is for core: #80855: Add element #type table and merge tableselect/tabledrag into it

moshe weitzman’s picture

#26 by lewisnyman is indeed a great responsive implementation but my sadness is that it gives up the bulk operations for narrow screens. If we could refresh the demo page with the checkboxes persisting, I'd say that we are good to proceed. I'd even give it a shot at implementation.

LewisNyman’s picture

Hey Moshe. The way I want to handle this is to The way I want to handle it is to a button that activates "bulk edit" mode which makes the checkboxes appear. Then we can make the whole "row" clickable for the checkbox.

This feature is actually in the nav.d8mux.org prototype navigate down to a list page like content->nodes and there's a button that looks like a list.

Is it better to separate that into a new issue or can we tackle it in here.

moshe weitzman’s picture

Assigned: Unassigned » moshe weitzman

OK, I see that list icon now. I'm hoping to try to implement this. I have no idea if I will succeed. There is a tricky problem in that list icon here depends on the new toolbar which is a different, larger patch.

dodorama’s picture

What if we make it possible to serve different markup to mobile devices rather than just try to bend the table using CSS ?
I feel like there should be something happening in the server rather than just do all the work client side.
For reference there's this interesting approach where feature/device detection is coupled with the MVC model where different views are served to different devices.
http://www.html5rocks.com/en/mobile/cross-device/
I feel like not everything can be solved with media queries and CSS/JS. Markup needs to be different in some cases.

Additional Resources
An overview of the hybrid approach client-side server-side
http://www.slideshare.net/4nd3rsen/ress-responsive-design-server-side-co...

A server side script
http://www.brettjankord.com/2012/01/16/categorizr-a-modern-device-detect...

LewisNyman’s picture

@36
How about just implementing the bulk
Edit mode using an action link? I hope that the contextual icons in te
Toolbar will be graphical representations on action links when we implement.

@37
Totally agree this will be the way forward, either through
Server side context or client side templating. BUT if we make this issue depandant on that we kill any progress for 6 months. We should create a separate issue.

gmclelland’s picture

For what it is worth, I did create a jquery plugin for Responsive Tables - http://jsfiddle.net/gmclelland/wAbuW/ Try resizing the browser to 320px and watch the table collapse even in IE9.

moshe weitzman’s picture

Lewis' prototype in #35 loses the 'select all' checkbox that all our tableselect elements have in the upper left. Could we get that feature into the prototype, somehow?

Similarly, when you check a checkbox in the prototype, you navigate to the detail page for that item. Drupal 7 doesn't do that, and instead does color the selected row in yellow.

Would be helpful to note in the source or elsewhere what CSS changes (and markup if possible) were made to get the behavior in the prototype.

nod_’s picture

@lewisnyman any news on a patch?

LewisNyman’s picture

Ok guys here we go, the php code is bad, the html is bad, the css is bad... but it's a start!

LewisNyman’s picture

I've fixed and expanded on the functionality in the static prototype based on the comments in #40

moshe weitzman’s picture

Status: Active » Needs work

In the static prototype, I can't check the checkboxes.

As for the patch, implementing seven_table() might not be the best way to go. That overrides all tables, not just tableselects which are the tables that have checkboxes in left column. Not sure what your intention is. Maybe you do want this same code for all tables on the admin theme?

nod_’s picture

well we talked about a proper way to do this earlier, I asked him to send a patch of his implementation to have something to work on since I wanted to start adapting the tableheader script and the final HTML is good enough to start working with it, regardless of how this is implemented on the backend.

So it's one way of doing it. That's not the best one and we should look at #33 for that, agreed.

RobW’s picture

Adding DOM manipulation to mobile browsers, especially manipulation of multiple nodes or manipulation recalculated on events, can cause serious performance hits (or if done really poorly, lock the page and crash it outright). We need to make sure that any fix to the table width problem actually improves mobile experience in the real world. A .js based responsive table script needs to put js performance at the forefront, be tested across devices, and have a configuration option to give developers and site builders the choice to use it or not.

@ #37:

Detector (http://drupal.org/project/detector) does feature testing coupled with server side UA feature caching. In other words, it lets you serve different templates based on browser and device capabilities. It's still a young project, but the Detector library developer Dave Molson is awesome, and it's always getting better.

Personally I think that with at the level of complexity of responsive tables, a project like Detector gives better overall performance and experience to the end user. But RESS in general is a newer paradigm, and not likely to make it's way into core any time soon.

nod_’s picture

You might want to try out the demo, this is a CSS only solution we're talking about.

LewisNyman’s picture

It is also mobile first.

nod_’s picture

FYI: I have Thursday and Friday reserved to work on the issue, any info to get me started on making that core worthy as #33 pointed out is welcome :)

RobW’s picture

Ah, sorry! When I was reading through the patch I misread some PHP as js. Entirely my fault, I blame the lack of caffeine. You guys are awesome.

Bojhan’s picture

Assigned: moshe weitzman » Unassigned
Status: Needs work » Needs review

Lets start reviewing this code, and all work on improving it. I am unassigning and marking it back to needs review, so more people look at it.

Status: Needs review » Needs work

The last submitted patch, drupal-responsivetables-1276908-42.patch, failed testing.

Kiphaas7’s picture

Accessibility wise, I am not sure this is the way to go (converting tables in unordered lists). Tables have a strong connection between the header element and the cells in a row.

I seriously think that a humongous unordered list with only span elements (span has no semantic value) is a huge regression for those using screen readers.

It would make sense to use a list (be it unordered, ordered or maybe even a definition list) if you'd only show 1 or 2 "cells". That would mean switching between tables and lists if you have more than 2 cells. However:

  1. as nod_RobW pointed out, doing DOM manipulation is costly, especially on mobile, even worse when doing on a large number of elements. Part of that could be solved with virtual scrolling, but that is just too much of a hassle to implement as a general solution for core.
  2. I think it's feasible to show at least 3 cells as a bare minimum while still keeping it mobile friendly. So no point in using lists.

I'd therefore very much +1 the general approach in #4. Keep using tables, give indication classes with PHP, collapse with CSS only.

LewisNyman’s picture

I don't think it's fair to compare <table> mark up to <span> mark up. Of course there is no semantic value in <span>.

Let's bear in mind that we can change the mark up to suit on a per page basis. What if we used <h3>, <section> and other appropriate elements?

Kiphaas7’s picture

I honestly don't know if that would would improve the semantics (and would have a similar strong relation between columns as tables do). Obviously the first question we need to ask is: what are we showing on most pages? Is it really tabular data? If it is, we should keep using tables.

Let's turn the question around: why do we even want to change from table to list? because the technique shown would be just as applicable for tables.

LewisNyman’s picture

why do we even want to change from table to list? because the technique shown would be just as applicable for tables.

Great question:

  1. The technique shown is not technically applicable in reverse. Internet Explorer in particular does not play well if you try to mould table rows and cells into a different layout.
  2. Starting with a list and expanding out into a table-like layout is a mobile first approach. This gives us a default that is automatically compatible with less capable, small screen devices.
Kiphaas7’s picture

Regarding point #1:
If I remember correctly, IE6-8 do not support media queries which would make the discussion of them not being able to mould table rows and cells into a different layout irrelevant at this point (mobile and IE6-8?). However, if IE9 and 10 have serious issues with this, I'd be really interested. From a quick search on google, I couldn't find any related issues. If you do know of them, please show me! The only thing I could find was this from the link in #2:
https://github.com/filamentgroup/RWD-Table-Patterns/commit/2236c9ec7b5af...

Regarding point #2:
I'm having trouble parsing this.

The exact same amount of html/data/text is present: each span equals a td, each li represents a tr, ul a table. Something similar for the header. So you can't possibly mean excessive markup?

Please, let me know why starting with a list is a mobile first approach. Note that this is not meant sarcastic in any way, I am genuinely curious why starting out with a list is preferred so strongly over a table.

RobW’s picture

+1 to Kiphaas7's point: If the data is tabular, put it in a table. If it's a one column ordered or unordered list, put it in a list.

If the solution being discussed is putting currently tabular content in lists, then the issue title should be changed to "Replace Tables with Lists" instead of "Fix Tables on Small Screens".

RobW’s picture

Unfortunately I don't have time to contribute anything really helpful here right now, so I'm going to bow out. Many thanks to Lewis N. and all others for working on this.

On my way out let me mention two small screen responsive methods I haven't seen here yet. Both work without data-attributes and hiding, and with the full table markup:

http://www.zurb.com/article/982/a-new-take-on-responsive-tables
http://dbushell.com/2012/01/05/responsive-tables-2/

Good luck, and I'm looking forward to what everyone here decides on.

JohnAlbin’s picture

Assigned: Unassigned » moshe weitzman

FYI, Moshe and Jesse Beach had a nearly-complete patch for this yesterday. Can't wait to see it.

Everett Zufelt’s picture

Transforming tables (those that contain tabular data) into lists is likely not going to fly from an accessibility perspective. AFAIK most admin tables carry the proper semantics. I'm at drupalcon so ping me / grab me and we can work through this and find a solution.

nod_’s picture

They used tables, no major markup changes planned.

jessebeach’s picture

Moshe and I worked on this in Munich Mobile sprint. This patch implements the "Maggie" approach discussed in [#1276908-2">#4. We implemented http://filamentgroup.com/lab/responsive_design_approach_for_complex_mult... but chose to simplify the UX with a single link to 'Show all columns' instead of a Column selector widget. The Show all columns toggle is smart enough to show only when there are hidden columns.

So, there is a new, simple API for callers to theme_table(). Each $header can now add an RWD_ADVISABLE (RWD is an common acronym for Responsive Web Design) class or a RWD_HELPFUL class. Both of these are hidden for narrow devices; adviseable shows up for tablet widths, and helpful columns show up for desktop widths. Columns with no responsive class are assumed to be ssential and always shown. See node.admin.inc and others in this patch for a use example.

We went back and forth about where to put the CSS and JS for responsive tables. We ended up putting the CSS in the individual themes and javascript in /misc because it functions independently of the responsiveness of a table. This decision may be revisited once we have system-wide breakpoints in core.

jessebeach’s picture

Status: Needs work » Needs review
attiks’s picture

Status: Needs review » Needs work

This looks great, I did a quick review (untested), only 'strange' thing on first site is the removal of display: none;
I assume there's no other way to handle this?

+++ b/core/misc/drupal.jsundefined
@@ -122,6 +122,40 @@ Drupal.checkWidthBreakpoint = function (width) {
+ * Returns a function that will not invoked whle it continues to be called,

whle => while

+++ b/core/misc/tableresponsive.jsundefined
@@ -0,0 +1,142 @@
+   * access all columns, however. This Class adds a toggle to a table with hidden

access to all ...

+++ b/core/misc/tableresponsive.jsundefined
@@ -0,0 +1,142 @@
+   * guiding principle of responsive design.

a guiding ...

+++ b/core/misc/tableresponsive.jsundefined
@@ -0,0 +1,142 @@
+    // Attach a resize hanlder to the window to check when

... handler

looks like there's also a part missing.

+++ b/core/misc/tableresponsive.jsundefined
@@ -0,0 +1,142 @@
+   * Associates an action link with the table will show hidden columns. Columns are assumed

... that will show the ...

+++ b/core/misc/tableresponsive.jsundefined
@@ -0,0 +1,142 @@
+    // if the table has hidden columns, associated an action link with the table

associate an action

+++ b/core/misc/tableresponsive.jsundefined
@@ -0,0 +1,142 @@
+    // interacted with it. This is the necessary to keep the link visible if the user

this is necessary ...

+++ b/core/misc/tableresponsive.jsundefined
@@ -0,0 +1,142 @@
+          var isDisplayNone = /^display\s*\:\s*none$/.exec(prop);

for performance reasons it might be better the generate the regexp only once?

attiks’s picture

I tested the patch, but something strange happens on field admin page if i click 'Show all columns'

Both weight and parent ar shown as well

Article | resp.local - Chromium_002.png

attiks’s picture

'Show all columns' and 'Show row weights' are on a different line, it would be better to put them on the same line I think.

Kiphaas7’s picture

Looks very promising!

+++ b/core/misc/drupal.jsundefined
+Drupal.debounce = function (fn, wait, immediate) {
+  var timeout;
+  return function () {
+    var context = this;
+    var args = arguments;
+    var deferred = function () {
+      timeout = null;
+      if (!immediate) {
+        fn.apply(context, args);
+      }
+    };
+    var invoke = immediate && !timeout;
+    clearTimeout(timeout);
+    timeout = setTimeout(deferred, wait);
+    if (invoke) {
+      fn.apply(context, args);
+    }
+  };
+};

I don't know about this: granted throttle and debounce functions aren't exactly rocket science anymore, but why introduce our own debounce (without throttle) if there are separate scripts out like https://github.com/cowboy/jquery-throttle-debounce/blob/master/jquery.ba...? There might also be some merit into having it into a separate file since this might actually be used a lot throughout core and contrib, which might not necessarily would want to load drupal.js.

+++ b/core/misc/tableresponsive.jsundefined
@@ -0,0 +1,142 @@
+    // Hide revealed columns.
+    else {
+      this.$revealedCells.hide();
+      this.$columnToggle.text(this.showText);
+      // Strip out display
+      this.$revealedCells.each(function (index, element) {
+        var $cell = $(this);
+        var style = $cell.attr('style');
+        var properties = style.split(';');
+        var newProps = [];
+        // The columns should simply have the display table-cell property
+        // removed, which the jQuery hide method does. The hide method
+        // also adds display none to the element. The element should be
+        // returned to the same state it was in before the columns were
+        // revealed, so it is necessary to remove the display none
+        // value from the style attribute.
+        for (var i = 0; i < properties.length; i++) {
+          var prop = properties[i]
+          prop.trim();
+          // Find the display:none property and remove it.
+          var isDisplayNone = /^display\s*\:\s*none$/.exec(prop);
+          if (isDisplayNone) {
+            continue;
+          }
+          newProps.push(prop);
+        }
+        // Return the rest of the style attribute values to the element.
+        $cell.attr('style', newProps.join(';'));
+      });
+      delete this.$revealedCells;
+      this.$columnToggle.data('drupal-tableresponsive').sticky = false;
+      // Refresh the toggle link.
+      $(window)
+      .triggerHandler('resize.drupal-tableresponsivetable');
+    }
+  };

3 questions pop into my head when I see this: Wasn't it possible to set it back to display:default? Or even better, toggle a drupal-cell-hidden class and do the hiding/showing in CSS based on this class? Or even better(derderder): is this all going down the drain once we have breakpoints? In that case, might add an @todo reminding us about that.

nod_’s picture

Pretty cool :D

Got a few comments:

  1. debouce in its own file++
  2. defining tableresponsive as a library depending on the debounce file
  3. any way to put the library in a preprocess for table and add a [#attached][library] entry to the render array?
  4. the regex thing looks ugly, can't we get rid of the $().hide() it comes from?
BrockBoland’s picture

Needs issue summary

moshe weitzman’s picture

Issue tags: -Needs issue summary update
FileSize
23.41 KB

Jesse and I implemented all the feedback except:

  1. Show row weights should be on same line as show all columns
  2. #69.4 the regex thing looks ugly, can't we get rid of the $().hide() it comes from?We plan to use drupal-cell-hidden class as per #68.

Jesse will handle those soon. Meanwhile, feel free to keep reviewing.

Issue summary updated.

nod_’s picture

nice :)

Already need reroll though.

  • use .attr() instead of the $(el, {})jQuery shortcut
  • $i = 0; foreach ($cells as $cell) { $i++;} can be replaced by foreach ($cells as $i => $cell) {} no?
  • missing "use strict"; in debounce.

tableresponsive code is very tabledrag-y that could use some makeup to make it a bit prettier :)

Kiphaas7’s picture

$i = 0; foreach ($cells as $cell) { $i++;} can be replaced by foreach ($cells as $i => $cell) {} no?

If they aren't associate arrays (is that a safe assumption here?) and if you can work around the fact that they immediately start at 0 instead of 1 (i++ is one of the first lines in that foreach).

Bojhan’s picture

Can anyone explain me the UX rationale behind plain out removing columns? I thought the whole idea of creating a responsive design is not to do any of the 'hide 'n cry' patterns also does this add a useless link on the desktop? Do we potentially confuse users with this?, the show-hide drag and drop links aren't really successful from a usability perspective. Doing just display;none on higher viewports only moves the problem to screenreader users.

mgifford’s picture

I came across this post by Jason Kiss recently (although it's a year old already):
http://www.accessibleculture.org/articles/2011/08/responsive-data-tables...

This option doesn't work with VoiceOver https://twitter.com/jkiss/status/240192146764873729 but again it's something that could be extended:
http://www.zurb.com/playground/playground/responsive-tables/index.html

Responsive accessibility is an interesting challenge.

nod_’s picture

I don't think there is a UX reason to it. It's technically easier and less intrusive. The list approach is potentially easier to keep all the data displayed at any resolution but it didn't get much agreement on the sprint the weekend before drupalcon.

The question is, what's your recommendation? It's just not possible to keep all the columns and make that show up nicely on mobile. What's the best trade-off UX-wise?

moshe weitzman’s picture

@Bojhan - the 'show all columns' link is only shown when columns have been hidden so no, there is no useless link added to desktop. Responsive is a bit more subtle than 'never hide any detail'. Lots of sites choose to show carefully curated thumbnail images on small screens instead of just scaled down versions of large images. These sites often hyperlink the thumbnail to the full version. We are doing same here - showing the most important part of the table and providing a quick and easy link to show it all. Showing all does not require a page refresh - it is instant.

In the responsive page navigation world, I've seen this called the Priority pattern: http://justmarkup.com/log/2012/06/19/responsive-multi-level-navigation

Also, I emphasize that themes are in control here. Modules call theme_table() with hints about the importance of each column. Core themes will choose to hide unimportant columns via media queries but other themes can choose other treatments or they can just ignore the hints entirely.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
41.75 KB

I added "use strict" to the debounce function.

tableresponsive.js and tabledrag.js now add their table controls through an API in the new table.js file. This API could be extended in the future to allow controls to be removed as well.

I left the code in that removes the display:none regexing. The problem with using a class (instead of $.show and $.hide) to expose the table columns, is the display property of a table cell must be set to table-cell in newer browsers and block in older browsers i.e. IE. And I didn't want to introduce logic to detect support of table-cell, which jQuery seems to do with the $.show method. The regex to pull out the display:none might offend the senses of some, but it's quite legit if not completely pleasing to one's sense of aesthetics.

I got a couple merge conflicts in the styling of Seven's style.css and tableheader.js. I'm hoping the patch applies cleanly. I rebased to the tip of 8.x HEAD before rolling the patch.

nod_’s picture

Status: Needs review » Needs work
  1. There are extra commas at the end of several objects, they need to be removed.
  2. Please use .attr() and not the jQuery shortcut.
  3. I really don't want this to catch on.
    newActions = $().add(newActions).add(link);
    
  4. What's the need for using .data() all over the place? Anything you can't do with object properties?
  5. The Action thing is pretty neat, I'd see that more as a constructor by itself than a method on the table object. Kinda out of scope though.
  6. The API could use some work:
    this.$columnToggle = this.table.addAction([this.actionLink]);
    

    So I'll get a jQuery object (I'm guessing a single element since Toggle is singular) by calling addAction on this.table, this.table that I'd expect to be a regular DOM object (since that's what it's named and used for is in tabledrag).
    Then I'm putting stuff in the $columnToggle.data(), if it represent several elements, where is the data actually stored? for each element, for the set, or just for the first element?
    So yeah, that's a pretty weird line if you ask me.

  7. in eventhandlerEvaluateColumnVisibility there are quite a lot of variables used only once, not really helping readability.
  8. var position = Number($header.prevAll('th').length); Not sure how that can't be a number.
  9. The "use strict"; could be added inside the debounce function, no need for the closure.
  10. eventhandlerToggleColumns seems like an expensive function, is it possible to look into using the col tag to do that, visibility:collapse on a col tag should do the trick here.
  11. $toggle
    .hide()
    .attr('aria-disabled', 'true');

    Can this be indented like the rest of core js?

    $toggle
      .hide()
      .attr('aria-disabled', 'true');
  12. not a fan of the new table dependency, wouldn't mind if it was simpler though.
  13. Possible to have the [#attached] thing in a new preprocess function for tables? because here, that's impossible to override (same goes for sticky actually).
  14. and in several js files: missing or extra comma, too many variables only used once,

Since the actual hiding/showing is done by CSS (except for the toggle case), I don't get what's more complicated than the mediaTable plugin which is 170 lines of JS (when you remove the generous spacing) which is mostly used to build checkboxes, and here we have no checkboxes, 150 lines for tableresponsive.js and some 70 for table.js

I'd rather just add the toggle link like we're currently doing for row weight, add a wrapper if necessary. The patch is for responsive table, not killer buttons.

(edit) just tried removing all the JS, the hiding/showing of columns still works. Not worth all the JS, the current patch is trying to do too much.

nod_’s picture

Looked more closely into using col and colgroups to do this thing, not really working as well as I'd expected :(

nod_’s picture

Issue summary: View changes

Updated issue summary.

nod_’s picture

That's more what I had in mind. Removed the table action api and debounce as well. That's useful on it's own and should have it's own issue.

Still need work, at least we're only left with the JS we need to have and don't sneak in some features at the same time.

nod_’s picture

Status: Needs work » Needs review

NR for attention, still NW.

Status: Needs review » Needs work

The last submitted patch, core-js-responsive-admin-1276908-81.patch, failed testing.

Grayside’s picture

Advisable and helpful are overly synonymous. Are they common terms I don't know?

alexpott’s picture

Disclaimer: not reviewed the patch at all.

But in trying to be helpful and provide advice about the point @Grayside was making in #84... I spotted...

+++ b/core/includes/theme.incundefined
@@ -34,6 +34,16 @@ const MARK_NEW = 1;
 /**
+ * A responsive table class.
+ */
+const RWD_ADVISABLE = 'advisable';
+
+/**
+ * A responsive table class.
+ */
+const RWD_HELPFUL = 'helpful';

These constant names look neither advisable or helpful :D ... well they might be if there was more commentary about how they are used.

nod_’s picture

patch was crap, reroll.

nod_’s picture

Status: Needs work » Needs review
Kiphaas7’s picture

+  /**
+   * Reveal hidden columns and hide any columns that were revealed because they were
+   * previously hidden.
+   */
+  eventhandlerToggleColumns: function (e) {

That doc is a bit of a WTF for those not following this issue. Isn't that a convoluted way of saying you want to toggle visiblity based on breakpoints?

nod_’s picture

oh yeah, the comments and all totally need work, I just wanted some opinion on the reduced-size patch which does the same in a simpler way. Feel free to update all that if you're up for it :)

Kiphaas7’s picture

Ah ok. So for now, ignore the following since it is more nitpicking ;)

+  // Attach a resize handler to the window.
+  $(window)
+    .on('resize.drupal-tableresponsive', $.proxy(this, 'eventhandlerEvaluateColumnVisibility'))
+    .trigger('resize.drupal-tableresponsive');
+}

Camelcase: resize.drupalTableResponsive? Or resize.drupalTableresponsive...

jessebeach’s picture

I rerolled the patch after commit #362889: Move drupal_common_theme() from common.inc into theme.inc rendered it invalid.

@Kiphaas7 I addressed your concerns in #88 and #90, please verify for me.

@nod_ I normalized the event and data namespaces according to https://drupal.org/node/1778828

nod_’s picture

Beside the fact that advisable and helpful are not really documented so we can't really know what they mean/do, I'm happy with it :)

nod_’s picture

it also need a small description of the features for people to test on this page: http://drupal.org/node/1777342. No idea if it's even possible to automate with testswarm.

jessebeach’s picture

Re: #93, I added rows for tableresponsive.js to http://drupal.org/node/1777342

moshe weitzman’s picture

FileSize
22.37 KB

per #92, I added Doxygen above the two new constants in theme.inc

nod_’s picture

Minor modifs, RTBC for me.

nod_’s picture

there was one ",)," leftover

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

The diff in #1276908-97: Administrative tables are too wide for smaller screens looks good. Let's get this in. Setting to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Here's an initial code review; haven't played with this functionality yet on my browser/phone.

+++ b/core/includes/theme.inc
@@ -34,6 +34,20 @@ const MARK_NEW = 1;
 /**
+ * A responsive table class. Indicates that a column has medium priority and thus
+ * can be hidden on narrow width devices and shown on medium+ width devices
+ * (i.e. tablets and desktops).
+ */
+const RWD_ADVISABLE = 'advisable';
+
+/**
+ * A responsive table class. Indicates that a column has low priority and thus
+ * can be hidden on narrow and medium viewports and shown on wide
+ * devices (i.e. desktops).
+ */
+const RWD_HELPFUL = 'helpful';

Do "advisable" and "helpful" come from some kind of spec, or did we make up these words to get across their meaning? If so, can we call them RWD_LOW and RWD_MEDIUM? I've so far seen no one who's come to "advisable" and "helpful" and intuitively understood what those meant.

(nitpick) The first line of a PHPDoc comment should be single <= 80 chars sentence; longer description can follow on a new paragraph.

+++ b/core/includes/theme.inc
@@ -1894,13 +1919,25 @@ function theme_table($variables) {
+    $i=0;

(nitpik) spaces on either side of the operator ($i = 0)

+++ b/core/misc/tabledrag.js
@@ -50,6 +50,7 @@ Drupal.tableDrag = function (table, tableSettings) {
   // Required object variables.
+  this.$table = $(table);
   this.table = table;

Both $table and table? Egad. :) Is this some JS thing I don't understand again, or could we rename one of them so it's more obvious what the differences are? I'm worried that makes the code incredibly hard to grok.

However, if JS people will understand this construct natively, please ignore.

+++ b/core/misc/tableresponsive.js
@@ -0,0 +1,149 @@
+ * tableresponsive.js

(nitpick) Need a @file declaration here.

+++ b/core/misc/tableresponsive.js
@@ -0,0 +1,149 @@
+/**
+ * A responsive table hides columns at small screen sizes, leaving the most
+ * important columns visible to the end user. Users should not be prevented from
...
+/**
+ * Extend the TableResponsive function to include an array that will list the
+ * tables currently being managed.
...
+  /**
+   * Associates an action link with the table that will show hidden columns.
+   * Columns are assumed to be hidden if their header is has the class helpful
...
+  /**
+   * Toggle the visibility of columns classed with either 'helpful' or
+   * 'advisable'.

(nitpick) Same as above about comment formatting standards. (first 1-sentence < 80 chars, rest in para below)

+++ b/core/modules/comment/comment.admin.inc
@@ -73,9 +73,9 @@ function comment_admin_overview($form, &$form_state, $arg) {
+    'author' => array('data' => t('Author'), 'field' => 'name', 'class' => array(RWD_ADVISABLE)),
+    'posted_in' => array('data' => t('Posted in'), 'field' => 'node_title', 'class' => array(RWD_HELPFUL)),
+    'changed' => array('data' => t('Updated'), 'field' => 'c.changed', 'sort' => 'desc', 'class' => array(RWD_HELPFUL)),

If I have a module that wants to change the RWD values of these, what's the process? Looks like hook_form_alter()? Do we need anything more specific than that?

webchick’s picture

Status: Needs work » Needs review

Sorry, Dreditor marked that needs work. Still leaving at needs review for others.

Kiphaas7’s picture

Both $table and table? Egad. :) Is this some JS thing I don't understand again, or could we rename one of them so it's more obvious what the differences are? I'm worried that makes the code incredibly hard to grok.

Indeed a JS thing. any variable prefixed with $ is a jQuery collection, without the prefix, but otherwise the same name, a DOM collection. Think this is used a lot in the JS/jQuery world, but I'll defer to someone else to post some conclusive thoughts on it.

jessebeach’s picture

Yes @Kiphaas7, that's the convention. Well said.

Dave Reid’s picture

I also agree that advisable and helpful are very confusing terms, and I have no idea what the different between those two actually are without having to look them up. Why not just use classes 'priority-medium' and 'priority-low' rather than constants? We need something else than the current terms.

EDIT: Also if we have to use constants, can the beginning part make sense too? RESPONSIVE_SOMETHING rather than RWD_SOMETHING.

jessebeach’s picture

Rerolled incorporating comments from:

#99, @webchick
#103, @Dave Reid (thanks for the constant name suggestions. We never felt we painted that bikeshed well with "helpful" and "advisable").

RWD_HELPFUL -> RESPONSIVE_PRIORITY_LOW
.helpful -> .priority-low
RWD_ADVISABLE -> RESPONSIVE_PRIORITY_MEDIUM
.advisable -> .priority-medium

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Verified that tables still work. Back to RTBC.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

The JS looks pretty cool, but I'm only qualified to comment on the PHP. :-) This is mostly minor, but all-in-all, I think this warrants a "needs work".

+++ b/core/includes/theme.inc
@@ -1759,6 +1777,9 @@ function theme_breadcrumb($variables) {
+ *     - "class": An array of values for 'class' attribute. In particular,
+ *        less important columns should have 'adviseable' or 'helpful' values.

This still uses the old 'adviseable' and 'helpful' terms. Also, I think we sould reference the constants directly here (i.e. RESPONSIVE_PRIORITY_LOW vs. 'priority-low')

+++ b/core/includes/theme.inc
@@ -1838,6 +1860,13 @@ function theme_table($variables) {
+  // If columns are hidden, add a link to show the columns.
+  if (count($header) && $responsive) {

Sorry for being nitpick-y, but this comment is wrong. count($header) being non-zero does not mean that any columns are hidden. If you don't actually want to hide any columns it would be worthless to set $responsive to TRUE, but following that logic we might as well kill the entire count($header) condition?!

EDIT: I just saw below that we default $responsive to TRUE, so maybe that's why we check that header isn't empty?!

+++ b/core/includes/theme.inc
@@ -1894,13 +1923,25 @@ function theme_table($variables) {
+    $i = 0;
     foreach ($header as $cell) {
+      $i++;

I've seen above why we are not using the "foreach ($header as $i => $cell)" pattern, but that deserves a comment, so we don't "fix" it in the future. I had the same thought just now, but remembered reading about that above...

EDIT: The hunk below shows that this pattern already exists (without a comment) in the same function, so I guess it would be OK to punt on this one.

+++ b/core/includes/theme.inc
@@ -1894,13 +1923,25 @@ function theme_table($variables) {
+      // Track responsive classes for each column as needed.

This comment could be approved, IMO. I think something like "Track responsive classes for each column, so we can apply them to the other column cells below." (i.e. mention *why* we are doing this, which I found to be non-obvious to be honest.)

+++ b/core/includes/theme.inc
@@ -1894,13 +1923,25 @@ function theme_table($variables) {
+      if (!empty($cell['class']) && is_array($cell['class'])) {

Sorry if this was mentioned before, but why do we check that $cell['class'] is an array? It smells like baby-sitting broken code, but I may totally be wrong here.

+++ b/core/includes/theme.inc
@@ -1944,7 +1985,18 @@ function theme_table($variables) {
+          // Copy advisable/helpful class from header to cell as needed.

This also still uses the old "advisable" and "helpful" terms.

I also noticed two more-"meta"-y things:
1. It's quite a few comments up, but I read that previous versions of this patch didn't work together with the "show/hide weights column"-feature of tabledrag. Since, as far as I can see we're removing all "display: none;" from table columns, it seems that still could be the case, but, again, this could be total BS.

2. A whole lot of tables are already covered in this patch, but I don't know if there are more and some of the changes I find debatable (although I personally wouldn't object to any of them; debatable is meant literally here). Do we need a follow-up issues to revisit all core tables and discuss each one a bit more, or should we just wait for someone, if at all, to complain :-) ?

tstoeckler’s picture

Oh, sorry for forgetting to mention: THIS IS FRICKIN' AWESOME!!! To me, this one of the coolest patches of D8 so far. Awesome work @all!!! :-)

jessebeach’s picture

Status: Needs work » Needs review
FileSize
23.5 KB

I rerolled, addressing comments in

#106 by @tstoeckler
a few comments by nod_ in chat (copied below)

@tstoeckler re #106: "It's quite a few comments up, but I read that previous versions of this patch didn't work together with the "show/hide weights column"-feature of tabledrag. Since, as far as I can see we're removing all "display: none;" from table columns, it seems that still could be the case, but, again, this could be total BS." -- I addressed this issue by tracking the columns that are revealed, then only those columns and removing display:none from them.

// tableresponsive.js, line 98
if ($hiddenHeaders.length > 0) {
  $hiddenHeaders.each(function (index, element) {
    var $header = $(this);
    var position = $header.prevAll('th').length;
    self.$table.find('tbody tr').each(function () {
      var $cells = $(this).find('td:eq(' + position + ')');
      $cells.show();
      // Keep track of the revealed cells, so they can be hidden later.
      self.$revealedCells = $().add(self.$revealedCells).add($cells);
    });
    $header.show();
    // Keep track of the revealed headers, so they can be hidden later.
    self.$revealedCells = $().add(self.$revealedCells).add($header);
  });
  this.$link.text(this.hideText).data('sticky', 1);
}

then

this.$revealedCells.hide();
  // Strip the 'display:none' declaration from the style attributes of
  // the table cells that .hide() added.
  this.$revealedCells.each(function (index, element) {
    var $cell = $(this);
    var properties = $cell.attr('style').split(';');
    var newProps = [];
    // The hide method adds display none to the element. The element should
    // be returned to the same state it was in before the columns were
    // revealed, so it is necessary to remove the display none
    // value from the style attribute.
    var match =  /^display\s*\:\s*none$/;
    for (var i = 0; i < properties.length; i++) {
      var prop = properties[i];
      prop.trim();
      // Find the display:none property and remove it.
      var isDisplayNone = match.exec(prop);
      if (isDisplayNone) {
        continue;
      }
      newProps.push(prop);
    }
    // Return the rest of the style attribute values to the element.
    $cell.attr('style', newProps.join(';'));
  });
  this.$link.text(this.showText).data('sticky', 0);
  // Refresh the toggle link.
  $(window).trigger('resize.tableresponsive');
}

I've made all the comments extremely explicit. Perhaps a touch too much. I hope it explains the functionality sufficiently.

comments from @nod_

in node_admin_nodes, the last 'changed' array is missing a ","

+      'class' => array(RESPONSIVE_PRIORITY_LOW)
+    ,)
should be 
+      'class' => array(RESPONSIVE_PRIORITY_LOW),
+    ),

and

style.css #page {} is not alphebitized properly
padding, margin-right, margin-left, should be maring-left, margin-right, padding

tstoeckler’s picture

Great, that removes all of my concerns. Since this was RTBC before, I think it should be now, but I guess a final look by someone who actually understands the JS couldn't hurt.

@jessebeach: I was looking for something like the self.$revealedCells-tracking you posted, but the entire JS is totally above my paygrade, which is why I missed. Thanks for taking the time to explain this in such detail! Much appreciated.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Tests are still ok. Awesome patch is awesome.

webchick’s picture

Awesome, this looks great now! Due to the swarming of Acquians here, I'm going to let this sit for another ~24 hours or so before committing. If catch gets to it first, lovely. :)

In the meantime, I found some minor nit-picky comment stuff while reading through, and have updated the patch. I don't think any of this should be contentious so leaving at RTBC.

+++ b/core/includes/theme.inc
@@ -34,6 +34,24 @@ const MARK_NEW = 1;
+ * A responsive table class.
...
+const RESPONSIVE_PRIORITY_MEDIUM = 'priority-medium';
...
+ * A responsive table class.
...
+const RESPONSIVE_PRIORITY_LOW = 'priority-low';

These will both end up showing the same PHPDoc at api.drupal.org. Needs a bit more details.

I changed these to:

 * A responsive table class; hide table cell on narrow devices.

and

 * A responsive table class; only show table cell on wide devices.

respectively.

+++ b/core/includes/theme.inc
@@ -34,6 +34,24 @@ const MARK_NEW = 1;
+ * Indicates that a column has medium priority and thus
+ * can be hidden on narrow width devices and shown on medium+ width devices
+ * (i.e. tablets and desktops).
...
+ * Indicates that a column has low priority and thus
+ * can be hidden on narrow and medium viewports and shown on wide
+ * devices (i.e. desktops).

(nitpick) These don't wrap at 80 chars. Fixed.

+++ b/core/includes/theme.inc
@@ -1759,6 +1777,14 @@ function theme_breadcrumb($variables) {
+ *     - "class": An array of values for 'class' attribute. In particular,

for "the" 'class' attributed. Fixed.

+++ b/core/misc/tableresponsive.js
@@ -0,0 +1,150 @@
+ * @file tableresponsive.js
+ *
+ * Behaviors to facilitate the presentation of tables across screens of any
+ * size.

Uh. So clearly I'm on crack and JS files don't get @file declarations. (Or at least nothing else in misc/ seems to.) Sorry. :D Removed.

+++ b/core/misc/tableresponsive.js
@@ -0,0 +1,150 @@
+  /**
+   * Associates an action link with the table that will show hidden columns.
+   *
+   * Columns are assumed to be hidden if their header has the class priority-low
+   * or priority-medium.
+   */

Indented one too many spaces. Fixed.

+++ b/core/themes/bartik/css/style.css
@@ -1708,3 +1708,21 @@ div.admin-panel .description {
+ * Responsive tables

+++ b/core/themes/seven/style.css
@@ -477,6 +489,23 @@ tr td:last-child {
+ * Responsive tables

+++ b/core/themes/stark/css/layout.css
@@ -93,3 +93,21 @@ img {
+ * Responsive tables

(nitpick) Comments should end in a period. Fixed.

dasjo’s picture

i'm not fully up-to-speed with the current approach, but i wanted to mention that i like the approach that zurb takes:
http://www.zurb.com/playground/responsive-tables

the horizontal-scroll pattern within the table container makes sense to me and could be combined with what i understand as the current approach, optionally hide less important columns.

Everett Zufelt’s picture

Has the proposed solution been tested with keyboard only users, and screen-readers on desktop / mobile? Does the table respond to window size, or device detection (assuming the former)?

attiks’s picture

Status: Reviewed & tested by the community » Needs work

Patch no longer applies :/

+++ b/core/misc/tableresponsive.jsundefined
@@ -0,0 +1,144 @@
+    var sticky = +this.$link.data('sticky');

this looks strange, or is it just me?

nod_’s picture

Same as parseInt(this.$link.data('sticky'), 10); I don't trust jQuery's .data().

attiks’s picture

Isn't it more clear to right it like that?

nod_’s picture

Status: Needs work » Needs review
FileSize
22.33 KB

reroll with parseInt change.

Status: Needs review » Needs work

The last submitted patch, core-js-responsive-admin-1276908-117.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

unrelated fail.

nod_’s picture

Kiphaas7’s picture

Code review for the JS:

+  // Store a reference to the header elements of the table so that the DOM is
+  // traversed only once to find them.
+  this.$headers = this.$table.find('th');
+  // Build a link that will toggle the column visibility.
+
+  // Add a link before the table for users to show or hide weight columns.
+  this.$link = $('<a href="#" class="tableresponsive-toggle"></a>')
+    .attr({
+      'title': Drupal.t('Expose table cells that were hidden to make the table fit within a small screen.'),
+      'aria-disabled': 'false'
+    })
+    .on('click', $.proxy(this, 'eventhandlerToggleColumns'));

Why is the empty line there? And those 2 descriptions look awfully alike.

+    var sticky = parseInt(this.$link.data('sticky'), 10);
+    var hiddenLength = this.$headers.filter('.priority-medium:hidden, .priority-low:hidden').length;
+    // If the table has hidden columns, associate an action link with the table
+    // to show the columns.
+    if (hiddenLength > 0) {
+      this.$link.show().text(this.showText).attr('aria-disabled', 'false');
+    }
+    // When the toggle is sticky, its presence is maintained because the user
+    // has interacted with it. This is necessary to keep the link visible if the
+    // user adjusts screen size and changes the visibilty of columns.

So at first I thought sticky was a reference to a sticky table header, then I saw it was parsed as an int, then I saw the doc describing the toggle as sticky. Confusing in drupal land with sticky table headers. At least for me, because I'm not sure what is meant with sticky here.

After skimming through the code, apparently sticky means the toggle has been used and should stay visible. If that is true, sticky is an ambiguous name for the reasons mentioned. Could this be something like toggleUsed or something more descriptive? toggleUsed is a bad name as well, so something better than that...

Nitpicking: And why is .data() used here if in all the js rewrites so far this has been handled as this.sticky = bool in the prototype? Not that it really matters, but we can actually properly store booleans there, so no need for parseInt.

Everett Zufelt’s picture

+      'title': Drupal.t('Expose table cells that were hidden to make the table fit within a small screen.'),

I'm not sure where this link is placed, but should expose == show, to be more in synch with 'Show row weights'?

+      'aria-disabled': 'false'

Why are we setting this attribute? Are there cases where we have it = TRUE?

jessebeach’s picture

@Everett Zufelt, yes, I'll change the word expose to the word show to maintain consistency.

Regarding the aria-disabled attribute. The attribute is toggled between true and false. Here is the code.

eventhandlerEvaluateColumnVisibility: function (e) {
    var sticky = +this.$link.data('sticky');
    var hiddenLength = this.$headers.filter('.priority-medium:hidden, .priority-low:hidden').length;
    // If the table has hidden columns, associate an action link with the table
    // to show the columns.
    if (hiddenLength > 0) {
      this.$link.show().text(this.showText).attr('aria-disabled', 'false');
    }
    // When the toggle is sticky, its presence is maintained because the user
    // has interacted with it. This is necessary to keep the link visible if the
    // user adjusts screen size and changes the visibilty of columns.
    if (!sticky && hiddenLength === 0) {
      this.$link.hide().text(this.hideText).attr('aria-disabled', 'true');
    }
  },

Is aria-disabled the wrong attribute to use here? I realize it has a reversed logic in the sense that when the link is visible, the value of the aria-disabled attribute is false. One would expect a value of true when the link is visible and an attribute that allowed a value of true. Is there a better attribute to use to indicate that the link is visible and the hidden columns are visible?

Everett Zufelt’s picture

@jessebeach

Thanks for the explanation. When the link is not visible is it removed (e.g. display: none)? If so, then there is no need for aria at all. aria exists to augment the native semantics, screen-readers and other UAs respect display: none.

Is there somewhere that I can test the current recommendation?

Everett Zufelt’s picture

Follow-up, aria-disabled is meant for situations where a control is visible, but disabled. E.g. a slider that is disabled unless you select a checkbox.

Kiphaas7’s picture

@Everett: I believe the most recent patch can be tested on the "content types" admin page.

jessebeach’s picture

@Everett Zufelt, ah, that's good to know abut display:none and screen readers!

I tried to set up a demo site yesterday, but D8 was fighting me and I kept getting 500 errors because the function cache() was undefined. I'll try rebooting the site this afternoon. Right now I'm working on #1608878: Add CTools dropbutton to core because I need that patch committed to include in the work here.

jessebeach’s picture

@Kiphaas7 #121, I changed the word 'sticky' to 'pegged' to disambiguate it with the concept of "sticky headers".

@Kiphaas7 #121, I removed the erroneous content and the empty line.

@Everett Zufelt #123, I changed 'Expose' to 'Show'.

Everett Zufelt #123, I removed the 'aria-disabled' attributes.

I also jshinted the file and added 'Drupal' and 'window' to the IIFE.

No other changes were made in this patch.

Remember, we have months to make this code better and of course it could be better. But it's a great place to start.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the updates, Jesse. I manually tested this AGAIN and it still works.

nod_’s picture

All good for me too.

Kiphaas7’s picture

+1, though I have some concerns regarding the forcefully hiding/showing of columns using the button (performance wise), it's nothing that requires an api change and can thus happily be fixed in a followup.

(what I'm talking about as a reminder for myself: http://jsperf.com/force-column-vis)

webchick’s picture

Title: Administrative tables are too wide for smaller screens » Change notice: Administrative tables are too wide for smaller screens
Category: bug » task
Priority: Normal » Critical
Issue tags: +Needs change record

Excellent, thanks for all the hard work on this, folks! I think this might need some more follow-ups as Kiphaas7 mentions to tweak performance (and also possibly accessibility), but getting this main feature in before feature freeze I feel is important.

Looked over the code and all my previous feedback was addressed. Committed and pushed to 8.x. Thanks!

We'll need a change notice for these new properties so that module developers make use of them when they port their modules to D8.

webchick’s picture

Status: Reviewed & tested by the community » Active
moshe weitzman’s picture

Title: Change notice: Administrative tables are too wide for smaller screens » Administrative tables are too wide for smaller screens
Status: Active » Fixed

Change Notice created at [#1796238]

I opened an issue for Views to support this - #1796230: Support responsive tables.

There are likely additional tables in core that could support this. I will poke around.

webchick’s picture

Issue tags: +Spark

Retroactively tagging "Spark."

webchick’s picture

Uhhh. Please? :P

Kiphaas7’s picture

Issue tags: -Spark

continuation for #131: poked around to use colgroup /col in combination with visibility: collapse for toggling column visibilty, but webkit fails to support this... :(

That would have been the holy grail to get this working blazingly fast. DAMM YOU WEBKIT!

JohnAlbin’s picture

Issue tags: +Spark

restoring spark tag

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Shyamala’s picture

A list of admin pages that need review are listed at: #1876196: Handling admin pages with tables as a followup

xjm’s picture

Category: task » bug
Priority: Critical » Normal
Issue tags: -Needs accessibility review, -Needs change record, -Needs JavaScript testing

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

Issue summary: View changes

...fixed wrong/trimmed links to post numbers and external sources and a minor typo. Also wrapped things in and here and there.