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
- The 'Show row weights' link should be on the same line as the 'Show all columns' toggle link.
- #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.
Comment | File | Size | Author |
---|---|---|---|
#128 | 1276908_responsive-tables_128.patch | 23.55 KB | jessebeach |
#117 | core-js-responsive-admin-1276908-117.patch | 22.33 KB | nod_ |
#111 | 1276908_responsive-tables_111.patch | 24.49 KB | webchick |
#108 | 1276908_responsive-tables_108.patch | 23.5 KB | jessebeach |
#104 | 1276908_responsive-tables_104.patch | 22.72 KB | jessebeach |
Comments
Comment #1
JohnAlbinHere's an interesting technique for responsive data tables. http://css-tricks.com/9096-responsive-data-tables/
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedA 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.
Comment #3
jefflinwood CreditAttribution: jefflinwood commentedI'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.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #5
jefflinwood CreditAttribution: jefflinwood commentedI 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.
Comment #6
LewisNyman CreditAttribution: LewisNyman commentedHere'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.
Comment #7
yoroy CreditAttribution: yoroy commentedWell, 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!).
Comment #8
LewisNyman CreditAttribution: LewisNyman commentedAll these need to be converted to the same mark up as this page: http://example.d8mux.org/admin
Comment #9
yoroy CreditAttribution: yoroy commentedThanks!
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.
Comment #10
yoroy CreditAttribution: yoroy commentedWhat about
/admin/content
/admin/content/book
/admin/content/comments
/admin/content/book/*
Lets ignore blocks page for this for now :)
Comment #11
FreekyMage CreditAttribution: FreekyMage commentedSo 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:
tables with more columns (the ones that end on "fields" and "display" are very similar, but should be tested seperatly anyway):
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:
Not sure if this needs fixing:
Comment #12
xjmAwesome research. Thanks @FreekyMage!
Comment #13
yoroy CreditAttribution: yoroy commentedThis 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 :)
Comment #14
LewisNyman CreditAttribution: LewisNyman commentedAsk and you shall receive :) Hacky code is my forte:
http://tables.d8mux.org/?q=admin/content
Comment #15
webchickNice. :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.
Comment #16
LewisNyman CreditAttribution: LewisNyman commentedThis 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.
Comment #17
gmclelland CreditAttribution: gmclelland commentedI started researching it and I found a couple of possible solutions.
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
Comment #18
gmclelland CreditAttribution: gmclelland commentedA plugin was just created http://mobifreaks.com/coding/responsive-data-tables-jquery/
Comment #19
gmclelland CreditAttribution: gmclelland commentedI 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?
Comment #20
LewisNyman CreditAttribution: LewisNyman commentedWhat 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
Comment #21
gmclelland CreditAttribution: gmclelland commentedThe 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.
Comment #22
nod_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.Comment #23
gmclelland CreditAttribution: gmclelland commented@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.
Comment #24
nod_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.
Comment #25
gmclelland CreditAttribution: gmclelland commentedThe 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).
Comment #26
LewisNyman CreditAttribution: LewisNyman commentedWhat 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
Comment #27
nod_Oh wow, this look awesome lewisnyman. Seriously, just wow. And that will make the tableheader script for admin tables way, way easier.
Comment #28
gmclelland CreditAttribution: gmclelland commented@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.
Comment #29
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.
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).
Comment #30
gmclelland CreditAttribution: gmclelland commented@nod_
- 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?
- 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.
Comment #31
nod_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?)
(already pointed out to before, don't mind me)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.
Comment #32
LewisNyman CreditAttribution: LewisNyman commentedI'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.
Comment #33
sunNote 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
Comment #34
moshe weitzman CreditAttribution: moshe weitzman commented#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.
Comment #35
LewisNyman CreditAttribution: LewisNyman commentedHey 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.
Comment #36
moshe weitzman CreditAttribution: moshe weitzman commentedOK, 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.
Comment #37
dodorama CreditAttribution: dodorama commentedWhat 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...
Comment #38
LewisNyman CreditAttribution: LewisNyman commented@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.
Comment #39
gmclelland CreditAttribution: gmclelland commentedFor 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.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedLewis' 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.
Comment #41
nod_@lewisnyman any news on a patch?
Comment #42
LewisNyman CreditAttribution: LewisNyman commentedOk guys here we go, the php code is bad, the html is bad, the css is bad... but it's a start!
Comment #43
LewisNyman CreditAttribution: LewisNyman commentedI've fixed and expanded on the functionality in the static prototype based on the comments in #40
Comment #44
moshe weitzman CreditAttribution: moshe weitzman commentedIn 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?
Comment #45
nod_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.
Comment #46
RobW CreditAttribution: RobW commentedAdding 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.
Comment #47
nod_You might want to try out the demo, this is a CSS only solution we're talking about.
Comment #48
LewisNyman CreditAttribution: LewisNyman commentedIt is also mobile first.
Comment #49
nod_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 :)
Comment #50
RobW CreditAttribution: RobW commentedAh, 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.
Comment #51
Bojhan CreditAttribution: Bojhan commentedLets 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.
Comment #53
Kiphaas7 CreditAttribution: Kiphaas7 commentedAccessibility 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:
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.I'd therefore very much +1 the general approach in #4. Keep using tables, give indication classes with PHP, collapse with CSS only.
Comment #54
LewisNyman CreditAttribution: LewisNyman commentedI 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?Comment #55
Kiphaas7 CreditAttribution: Kiphaas7 commentedI 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.
Comment #56
LewisNyman CreditAttribution: LewisNyman commentedGreat question:
Comment #57
Kiphaas7 CreditAttribution: Kiphaas7 commentedRegarding 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 atd
, eachli
represents atr
,ul
atable
. 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.
Comment #58
RobW CreditAttribution: RobW commented+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".
Comment #59
RobW CreditAttribution: RobW commentedUnfortunately 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.
Comment #60
JohnAlbinFYI, Moshe and Jesse Beach had a nearly-complete patch for this yesterday. Can't wait to see it.
Comment #61
Everett Zufelt CreditAttribution: Everett Zufelt commentedTransforming 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.
Comment #62
nod_They used tables, no major markup changes planned.
Comment #63
jessebeach CreditAttribution: jessebeach commentedMoshe 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.
Comment #64
jessebeach CreditAttribution: jessebeach commentedComment #65
attiks CreditAttribution: attiks commentedThis 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?
whle => while
access to all ...
a guiding ...
... handler
looks like there's also a part missing.
... that will show the ...
associate an action
this is necessary ...
for performance reasons it might be better the generate the regexp only once?
Comment #66
attiks CreditAttribution: attiks commentedI 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
Comment #67
attiks CreditAttribution: attiks commented'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.
Comment #68
Kiphaas7 CreditAttribution: Kiphaas7 commentedLooks very promising!
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.
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.
Comment #69
nod_Pretty cool :D
Got a few comments:
[#attached][library]
entry to the render array?Comment #70
BrockBoland CreditAttribution: BrockBoland commentedNeeds issue summary
Comment #71
moshe weitzman CreditAttribution: moshe weitzman commentedJesse and I implemented all the feedback except:
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.
Comment #72
nod_nice :)
Already need reroll though.
$i = 0; foreach ($cells as $cell) { $i++;}
can be replaced byforeach ($cells as $i => $cell) {}
no?tableresponsive code is very tabledrag-y that could use some makeup to make it a bit prettier :)
Comment #73
Kiphaas7 CreditAttribution: Kiphaas7 commentedIf 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).
Comment #74
Bojhan CreditAttribution: Bojhan commentedCan 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.
Comment #75
mgiffordI 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.
Comment #76
nod_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?
Comment #77
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #78
jessebeach CreditAttribution: jessebeach commentedI 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 totable-cell
in newer browsers andblock
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 thedisplay: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.
Comment #79
nod_.data()
all over the place? Anything you can't do with object properties?So I'll get a jQuery object (I'm guessing a single element since Toggle is singular) by calling
addAction
onthis.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.
eventhandlerEvaluateColumnVisibility
there are quite a lot of variables used only once, not really helping readability.var position = Number($header.prevAll('th').length);
Not sure how that can't be a number."use strict";
could be added inside the debounce function, no need for the closure.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.Can this be indented like the rest of core js?
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.
Comment #80
nod_Looked more closely into using col and colgroups to do this thing, not really working as well as I'd expected :(
Comment #80.0
nod_Updated issue summary.
Comment #81
nod_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.
Comment #82
nod_NR for attention, still NW.
Comment #84
Grayside CreditAttribution: Grayside commentedAdvisable and helpful are overly synonymous. Are they common terms I don't know?
Comment #85
alexpottDisclaimer: 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...
These constant names look neither advisable or helpful :D ... well they might be if there was more commentary about how they are used.
Comment #86
nod_patch was crap, reroll.
Comment #87
nod_Comment #88
Kiphaas7 CreditAttribution: Kiphaas7 commentedThat 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?
Comment #89
nod_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 :)
Comment #90
Kiphaas7 CreditAttribution: Kiphaas7 commentedAh ok. So for now, ignore the following since it is more nitpicking ;)
Camelcase: resize.drupalTableResponsive? Or resize.drupalTableresponsive...
Comment #91
jessebeach CreditAttribution: jessebeach commentedI 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
Comment #92
nod_Beside the fact that
advisable
andhelpful
are not really documented so we can't really know what they mean/do, I'm happy with it :)Comment #93
nod_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.
Comment #94
jessebeach CreditAttribution: jessebeach commentedRe: #93, I added rows for tableresponsive.js to http://drupal.org/node/1777342
Comment #95
moshe weitzman CreditAttribution: moshe weitzman commentedper #92, I added Doxygen above the two new constants in theme.inc
Comment #96
nod_Minor modifs, RTBC for me.
Comment #97
nod_there was one ",)," leftover
Comment #98
jessebeach CreditAttribution: jessebeach commentedThe diff in #1276908-97: Administrative tables are too wide for smaller screens looks good. Let's get this in. Setting to RTBC.
Comment #99
webchickHere's an initial code review; haven't played with this functionality yet on my browser/phone.
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.
(nitpik) spaces on either side of the operator (
$i = 0
)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.
(nitpick) Need a @file declaration here.
(nitpick) Same as above about comment formatting standards. (first 1-sentence < 80 chars, rest in para below)
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?
Comment #100
webchickSorry, Dreditor marked that needs work. Still leaving at needs review for others.
Comment #101
Kiphaas7 CreditAttribution: Kiphaas7 commentedIndeed 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.
Comment #102
jessebeach CreditAttribution: jessebeach commentedYes @Kiphaas7, that's the convention. Well said.
Comment #103
Dave ReidI 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.
Comment #104
jessebeach CreditAttribution: jessebeach commentedRerolled 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
Comment #105
moshe weitzman CreditAttribution: moshe weitzman commentedVerified that tables still work. Back to RTBC.
Comment #106
tstoecklerThe 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".
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')
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?!
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.
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.)
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.
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 :-) ?
Comment #107
tstoecklerOh, sorry for forgetting to mention: THIS IS FRICKIN' AWESOME!!! To me, this one of the coolest patches of D8 so far. Awesome work @all!!! :-)
Comment #108
jessebeach CreditAttribution: jessebeach commentedI 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.then
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 ","
and
style.css #page {} is not alphebitized properly
padding, margin-right, margin-left, should be maring-left, margin-right, padding
Comment #109
tstoecklerGreat, 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.Comment #110
nod_Tests are still ok. Awesome patch is awesome.
Comment #111
webchickAwesome, 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.
These will both end up showing the same PHPDoc at api.drupal.org. Needs a bit more details.
I changed these to:
and
respectively.
(nitpick) These don't wrap at 80 chars. Fixed.
for "the" 'class' attributed. Fixed.
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.
Indented one too many spaces. Fixed.
(nitpick) Comments should end in a period. Fixed.
Comment #112
dasjoi'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.
Comment #113
Everett Zufelt CreditAttribution: Everett Zufelt commentedHas 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)?
Comment #114
attiks CreditAttribution: attiks commentedPatch no longer applies :/
this looks strange, or is it just me?
Comment #115
nod_Same as
parseInt(this.$link.data('sticky'), 10);
I don't trust jQuery's.data()
.Comment #116
attiks CreditAttribution: attiks commentedIsn't it more clear to right it like that?
Comment #117
nod_reroll with parseInt change.
Comment #119
nod_unrelated fail.
Comment #120
nod_#117: core-js-responsive-admin-1276908-117.patch queued for re-testing.
Comment #121
Kiphaas7 CreditAttribution: Kiphaas7 commentedCode review for the JS:
Why is the empty line there? And those 2 descriptions look awfully alike.
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 asthis.sticky = bool
in the prototype? Not that it really matters, but we can actually properly store booleans there, so no need for parseInt.Comment #122
Everett Zufelt CreditAttribution: Everett Zufelt commented+ '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?
Comment #123
jessebeach CreditAttribution: jessebeach commented@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.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 thearia-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?Comment #124
Everett Zufelt CreditAttribution: Everett Zufelt commented@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?
Comment #125
Everett Zufelt CreditAttribution: Everett Zufelt commentedFollow-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.
Comment #126
Kiphaas7 CreditAttribution: Kiphaas7 commented@Everett: I believe the most recent patch can be tested on the "content types" admin page.
Comment #127
jessebeach CreditAttribution: jessebeach commented@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.
Comment #128
jessebeach CreditAttribution: jessebeach commented@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.
Comment #129
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for the updates, Jesse. I manually tested this AGAIN and it still works.
Comment #130
nod_All good for me too.
Comment #131
Kiphaas7 CreditAttribution: Kiphaas7 commented+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)
Comment #132
webchickExcellent, 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.
Comment #133
webchickComment #134
moshe weitzman CreditAttribution: moshe weitzman commentedChange 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.
Comment #135
webchickRetroactively tagging "Spark."
Comment #136
webchickUhhh. Please? :P
Comment #137
Kiphaas7 CreditAttribution: Kiphaas7 commentedcontinuation 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!
Comment #138
JohnAlbinrestoring spark tag
Comment #140
Shyamala CreditAttribution: Shyamala commentedA list of admin pages that need review are listed at: #1876196: Handling admin pages with tables as a followup
Comment #141
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #141.0
xjm...fixed wrong/trimmed links to post numbers and external sources and a minor typo. Also wrapped things in
and here and there.