Problem/Motivation

When adding a text field with multiple values, the text field will overflow over its designated area on narrow screens (the minimum width this happens is 780px).

Proposed resolution

Port the solution from Claro, and change .form-element to .form-text

/**
 * Target every .form-element input that parent is a form-item of a table cell.
 * This ignores the filter format select of the textarea editor.
 */

td > .form-item > .form-element,
td > .ajax-new-content > .form-item > .form-element {
  width: 100%;
}

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shaal created an issue. See original summary.

shaal credited byrond.

shaal’s picture

shaal’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
524 bytes
334.28 KB

Screenshot after applying the patch:

shaal’s picture

Rerolled patch for 8.9.x (no interdiff)

shaal’s picture

Issue tags: +good-first-issue, +Novice
priyanka.sahni’s picture

Assigned: Unassigned » priyanka.sahni
priyanka.sahni’s picture

FileSize
306.86 KB
293.13 KB

Verified and tested by applying patch #5.It is working fine for D.8.9.Can be moved to RTBC.

Steps to test -
1. Go to the admin site.
2. Go to admin/content.
3. Search for the recipe content and edit it.
4. Verify

BeforePatch

After Patch

priyanka.sahni’s picture

Assigned: priyanka.sahni » Unassigned
shaal’s picture

@priyanka.sahni Thank you for reviewing the patch.
If the patch was working for you and did what it suppose to, you can change the status of this issue to RTBC.

shimpy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
83.08 KB
63.85 KB

Patch #5 looks good to me as working as per functionality. Here attaching screenshots for same and changing status to RTBC

Before patch
Before Patch

After Patch

After patch

lauriii’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/themes/seven/css/components/tables.css
@@ -150,3 +150,13 @@ th.select-all {
+  max-width: 62ch;

I'm wondering how does this play if someone has a text field that has size higher than 60 characters? I'm also concerned about some browsers not respecting max-width for form inputs.

Kristen Pol’s picture

Issue tags: +Global2020

Tagging this issue for new contributors to work on during the Friday DrupalCon Global sprint.

Kristen Pol’s picture

Version: 9.0.x-dev » 9.1.x-dev
rakhimandhania’s picture

Issue tags: +DIACWJuly2020
imalabya’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
419.15 KB
421.45 KB

Tested the patch in various browsers (Chrome, Firefox, Safari, Edge & IE11) Works well. The text fields doesn't overflow.
However, I don't think the max-width: 62ch is redundant. Also, in IE11 the text field doesn't take the full width of the container.

With max-width: 62ch

Without max-width: 62ch

IMO, we can remove the max-width: 62ch from the PR.

harika gujjula’s picture

Assigned: Unassigned » harika gujjula
harika gujjula’s picture

Assigned: harika gujjula » Unassigned
Status: Needs work » Needs review
FileSize
502 bytes

I have removed the max-width set and tested. This works fine.
Please find the updated patch.

Kristen Pol’s picture

@harika gujjula Thank you. When you update patches, it is best to include an interdiff:

https://www.drupal.org/documentation/git/interdiff

Though in this case it is ok since the patch is so small.

HarishST’s picture

Applied the patch in #18 and works as intended. Attached the screenshots by testing on Firefox, Chrome and IE11.
Thanks @harika, for the patch.
It looks good to me. Moving to RTBC.

yoroy’s picture

Status: Reviewed & tested by the community » Needs review

Tested this with a new multi-value text field item on the article content type. Works as described, so this seems like a good fix.

One niggle, this code comment reads a bit awkward, not sure it's super correct:

> Target every .form-text input that its parent is a form-item of a table cell.

Should that be something like

> Target every .form-text input where its parent is a form-item inside a table cell.

?

Kristen Pol’s picture

Status: Needs review » Needs work

Thanks for the reviews. Back to needs work for comment change.

Hardik_Patel_12’s picture

Status: Needs work » Needs review
FileSize
497 bytes
514 bytes

Updating the comment , kindly review.

samiullah’s picture

Comment has been added as expected
Target every .form-text input where its parent is a form-item inside a table cell. */

@kristen.pol this can be moved to RTBC if no other code review is needed
Thanks

Kristen Pol’s picture

Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative

Thanks for the update and testing. A couple nitpicks:

+++ b/core/themes/seven/css/components/tables.css
@@ -151,3 +151,9 @@ th.select-all {
+/* Target every .form-text input where its parent is a form-item inside a table cell. */

1) Exceeds 80 characters.

2) The comment style switched from:

/**
 * [comment]
 */

to

/* [comment] */

It appears the latter style is normally used for inline comments (after the CSS), or sometimes if the comment is only one line long. Since the comment exceeds 80 characters, if that was changed, it would be more than one line and should switch to the former format.

msuthars’s picture

Assigned: Unassigned » msuthars
msuthars’s picture

Assigned: msuthars » Unassigned
Status: Needs work » Needs review
FileSize
793 bytes
788 bytes

@Kristen Pol thanks for the suggestions. I updated the patch. Please review.

Kristen Pol’s picture

Status: Needs review » Needs work

Thanks for the update.

  1. +++ b/core/themes/seven/css/components/tables.css
    @@ -126,16 +126,12 @@ th.select-all {
    -/**
    - * Captions.
    - */
    +/* Captions. */
    

    Changes here are out of scope for this issue.

  2. +++ b/core/themes/seven/css/components/tables.css
    @@ -126,16 +126,12 @@ th.select-all {
    -/**
    - * Responsive tables.
    - */
    +/* Responsive tables. */
    

    Changes here are out of scope for this issue.

  3. +++ b/core/themes/seven/css/components/tables.css
    @@ -151,3 +147,10 @@ th.select-all {
    +/* Target every .form-text input where its parent is a form-item inside a table
    + cell. */
    

    This doesn't address the second part of the comment in #25:

    Since the comment exceeds 80 characters, if that was changed, it would be more than one line and should switch to the former format.

    If this is not clear, it needs to change to using this formatting:

    /**
     * [comment]
     */
    
ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
509 bytes
871 bytes

Here I have addressed comment #28.

Status: Needs review » Needs work

The last submitted patch, 29: 3151119-29.patch, failed testing. View results

ravi.shankar’s picture

Status: Needs work » Needs review

Changing status to needs review as patch #29 passed the tests.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the update.

1) Patch applies cleanly.

2) Tests pass.

3) Code was manually tested in #20 and #21 so I don't think it needs retesting. Subsequent changes were to comments.

4) CSS was reviewed in #12 and #16, and concerns were addressed by removing max-width: 62ch. Subsequent changes were to comments.

5) #29 addresses items from #28.

6) Although my inclination would be to try to get the text down to a one-line comment (since it spills over by only one word), the wording is what was suggested in #21. I'm not sure if this needs an automated test or needs testing on other themes, but marking RTBC to get maintainers thoughts.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: 3151119-29.patch, failed testing. View results

Hardik_Patel_12’s picture

Status: Needs work » Reviewed & tested by the community

Random failure , setting back to RTBC.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

alexpott’s picture

Title: Text field with multiple values overflows on narrow screens » [backport] Text field with multiple values overflows on narrow screens
Version: 9.2.x-dev » 9.1.x-dev

Committed c5489de and pushed to 9.2.x. Thanks!

Going to discuss with @lauriii about whether we should backport this to 9.1.x

  • alexpott committed c5489de on 9.2.x
    Issue #3151119 by shaal, Hardik_Patel_12, msuthars, ravi.shankar, harika...
alexpott’s picture

Title: [backport] Text field with multiple values overflows on narrow screens » Text field with multiple values overflows on narrow screens
Status: Reviewed & tested by the community » Fixed

Discussed with @lauriii we agreed to backport this to 9.1.x.

  • alexpott committed f579428 on 9.1.x
    Issue #3151119 by shaal, Hardik_Patel_12, msuthars, ravi.shankar, harika...

Status: Fixed » Closed (fixed)

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