In #2130321: Fatal errors when editing issues with large number of comments (300+?) - cannot be edited nor can comments be added the Javascript for forums pages and api.drupal.org runs to check if the table if bigger than the content area and then expands the content area to fit it.

We don't want it to run on any other pages apart from api and forum. That's what this patch does.

Known word-wrapping problems:

  • File fields on issues
  • Node changes on issues
  • Forum listings - currently always breaks mid-word
  • API.Drupal.org listings

Comments

drumm’s picture

Larger tables can happen all over the site, those are just reliable examples for finding potentially layout-breaking tables. Instead, we should make the tables more narrow. How about something like:

.extended-file-field-table-filename {
  max-width: 30em;
  text-overflow: ellipsis;
  overflow: hidden;
}

If #2225355: Make tables responsive is able to ensure tables don't break out of their container, then the JS can be removed completely.

drumm’s picture

Status: Needs review » Needs work
lewisnyman’s picture

@drumm Yeah we discussed this the other day, it possible that on some api.drupal.org pages the tables will collapse even on desktop 0_0

dddave’s picture

I guess this is happening here? https://drupal.org/node/2159541

drumm’s picture

The wide table in #2159541: Notice: Undefined property: stdClass::$title in flexslider_entityreference_field_formatter_settings_summary() is the title change in comment #2. It could be tackled in a similar way as I proposed for the files table in #1.

drumm’s picture

I noticed word-wrap: break-word; is used on issue queue tables. That actually looks ideal. I don't know if it needs to be combined with max-width or not.

If we can handle all tables with wide words with CSS, then the JS can be removed completely.

dddave’s picture

Mmmh, this just came up in the webmaster queue #2248067: Issue too wide not sure if same but different or just different.

drumm’s picture

We can tackle both types of table on issue pages in this issue. They will have similar CSS to deal with wide content.

dddave’s picture

Related issues: +#2248067: Issue too wide
drumm’s picture

word-wrap: break-word; should also be applied to URLs under the logo on organization pages, such as https://www.drupal.org/node/1237520.

lewisnyman’s picture

lewisnyman’s picture

Status: Needs work » Needs review

  • drumm committed caab18d on 7.x-1.x authored by LewisNyman
    #2228139 Table spacing JS runs on issue pages, occasionally breaking...
drumm’s picture

Title: Table spacing JS runs on issue pages, occasionally breaking layout » Remove table spacing JS
Issue summary: View changes

Great, let's use this issue to totally remove the table spacing JS.

drumm’s picture

Status: Needs review » Active
Issue tags: +needs drupal.org deployment
drumm’s picture

Issue summary: View changes
Issue tags: -needs drupal.org deployment

Now deployed.

drumm’s picture

Issue summary: View changes
markhalliwell’s picture

Status: Active » Needs work
StatusFileSize
new273.23 KB
+++ b/sass/partials/drupalorg/_issue-page.scss
@@ -128,6 +128,18 @@
+.field-name-field-issue-files table,
+.field-name-field-issue-changes table.nodechanges-file-changes {
+  max-width: 580px;
+}

The first ruleset is not specific enough. It also restricts the table inside the files fieldset at the bottom of issues:

Also, I'm not entirely sure why a max-width is used here. Surely there must be a better solution than this. At the very least, wouldn't max-width: 100% work instead of such a very specific and arbitrary pixel size?

lewisnyman’s picture

Here's a more specific selector that works without side effects:

.node-project-issue .field-name-field-issue-files table, 
.node-project-issue .field-name-field-issue-changes table.nodechanges-file-changes
At the very least, wouldn't max-width: 100% work instead of such a very specific and arbitrary pixel size?

It doesn't, I tried that first :'(

  • drumm committed 4eb1671 on 7.x-1.x
    #2228139 Clean up CSS rule for wide tables on issue pages.
    
drumm’s picture

Issue tags: +needs drupal.org deployment

#20 should fix #19.

drumm’s picture

Issue tags: -needs drupal.org deployment

  • drumm committed 4eb1671 on 7.x-2.x
    #2228139 Clean up CSS rule for wide tables on issue pages.
    
  • DyanneNova committed c8e0589 on 7.x-2.x
    Merge branch '7.x-1.x' into 7.x-2.x
    
    * 7.x-1.x: (72 commits)
    #2322267...
  • drumm committed caab18d on 7.x-2.x authored by LewisNyman
    #2228139 Table spacing JS runs on issue pages, occasionally breaking...
mgifford’s picture

So am I right in that these are the items which are outstanding:
- Node changes on issues - https://www.drupal.org/node/2056089/revisions
- Forum listings - https://www.drupal.org/forum/
- API.Drupal.org listings - https://api.drupal.org/api/drupal/groups

I'm not certain that those links are right, but wanted to have a quick way to evaluate.

I'm just not certain what we need to do to close this issue.

mgifford’s picture

duplicate

  • DyanneNova committed 3ab11e4 on 7.x-2.x
    Issue #2228139: Wrap words in issue node changes
    
drumm’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
Issue summary: View changes

For Node changes on issues, this is now not causing layout issues, but looks like it is always breaking mid-word. If possible, it should try to break on words, and break mid-word when necessary.

drumm’s picture

Issue summary: View changes

The same is true of forum listings.

drumm’s picture

Assigned: Unassigned » drumm

Retitling since the JS is gone, and we still have a couple places to clean up.

https://kenneth.io/blog/2012/03/04/word-wrapping-hypernation-using-css/ looks like a good strategy for wrapping long words in dynamically-sized containers. Removing the need to hard-code a width, as mentioned in #19.

https://drumm-drupal.redesign.devdrupal.org/node/2355909 is an example of this running right now.

drumm’s picture

Title: Remove table spacing JS » Improve line wrapping in tables

  • drumm committed 2322612 on 7.x-2.x
    Issue #2228139: Wrap long words on issue pages.
    
drumm’s picture

I am deploying these changes for issue pages shortly.

drumm’s picture

Issue summary: View changes

Now deployed.

  • drumm committed caab18d on 2228139-line-wrapping authored by LewisNyman
    #2228139 Table spacing JS runs on issue pages, occasionally breaking...
  • drumm committed 4eb1671 on 2228139-line-wrapping
    #2228139 Clean up CSS rule for wide tables on issue pages.
    
  • DyanneNova committed c8e0589 on 2228139-line-wrapping
    Merge branch '7.x-1.x' into 7.x-2.x
    
    * 7.x-1.x: (72 commits)
    #2322267...
  • DyanneNova committed e0b612c on 2228139-line-wrapping
    Issue #2228139: Wrap words on api
    
  • DyanneNova committed dcfb4fd on 2228139-line-wrapping
    Issue #2228139: Wrap words in forum titles
    
  • DyanneNova committed 3ab11e4 on 2228139-line-wrapping
    Issue #2228139: Wrap words in issue node changes
    
  • drumm committed 2322612 on 2228139-line-wrapping
    Issue #2228139: Wrap long words on issue pages.
    
  • drumm committed 4563014 on 2228139-line-wrapping
    Issue #2228139: Improve line wrapping within API.Drupal.org tables
    
  • drumm committed e946b94 on 2228139-line-wrapping
    Issue #2228139: Refactor responsive tables JS.
    
    - Always apply...
drumm’s picture

toboggan-api.redesign.devdrupal.org has the improved line wrapping in tables.

The table columns do get quite narrow, so I refactored the JS to kick in the mobile table styles when a full-width views table gets less than 500px wide on the site.

  • drumm committed f59566f on 2228139-line-wrapping
    Issue #2228139: Improve line wrapping in tables on forum pages
    

  • drumm committed 4563014 on 7.x-2.x, dev
    Issue #2228139: Improve line wrapping within API.Drupal.org tables
    
  • drumm committed e946b94 on 7.x-2.x, dev
    Issue #2228139: Refactor responsive tables JS.
    
    - Always apply...
  • drumm committed f59566f on 7.x-2.x, dev
    Issue #2228139: Improve line wrapping in tables on forum pages
    
drumm’s picture

Status: Needs work » Fixed
Issue tags: +needs drupal.org deployment

The last few commits fix this.

In http://cgit.drupalcode.org/bluecheese/commit/?id=e946b94, I refactored the JS to be a single file, and more flexible about when to make specific tables responsive.

drumm’s picture

Issue tags: -needs drupal.org deployment

Now deployed.

Status: Fixed » Closed (fixed)

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