Problem/Motivation
Issue #1276908: Administrative tables are too wide for smaller screens added totally amazing responsive table functionality to Drupal 8. The logic in the new tableresponsive.js contains a wildly elaborate approach to removing the display: none from the table cells.
From the original issue, it looks like @jessebeach had concerns about trying to set the display property *back* to display: block or display: table-cell:
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.
However rather than trying to set the display property to something else, it'd be much easier just to remove the display: none style using jQuery (http://api.jquery.com/css/).
Setting the value of a style property to an empty string — e.g. $( "#mydiv" ).css( "color", "" ) — removes that property from an element if it has already been directly applied, whether in the HTML style attribute, through jQuery's .css() method, or through direct DOM manipulation of the style property. It does not, however, remove a style that has been applied with a CSS rule in a stylesheet or
element.
Proposed resolution
Use $.fn.css() instead of custom code.Remaining tasks
Review.User interface changes
None.API changes
None.| Comment | File | Size | Author |
|---|---|---|---|
| #22 | 2277761-22.patch | 1.92 KB | Abhisheksingh27 |
| #9 | interdiff.txt | 1.86 KB | aj2r |
| #9 | 2277761-9.patch | 1.86 KB | aj2r |
| #1 | tableresponsive-easier-2277761.patch | 1.53 KB | quicksketch |
Comments
Comment #1
quicksketchComment #2
nod_Looks good, seems like tableresponsive.js isn't added to pages with responsive tables though…
Comment #3
droplet commentedCan we add back the code comments in above patch. It's the most important part of the code. Otherwise, calling hide() and then remove the display:none seems like a very stupid logic to everybody :)
Comment #4
nod_That works, and still applies. That said I agree with droplet, we need to keep the comments.
Comment #8
aj2r commentedComment #9
aj2r commentedRerolled to 8.4.x and preserved comments.
Comment #21
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
At this time we will need a D10 version of this issue.
This will need to be manually tested to verify that the tables are still working correctly and responsive.
Comment #22
Abhisheksingh27 commentedAdding reroll for 10.1.x as the previous patch failed to apply on D10.
please review
Comment #23
smustgrave commentedTested this manually on Drupal 10 with a standard install.
Going to the content view and going to mobile size the columns are hidden.
Triggering show and hide columns works as expected.