Problem/Motivation

I was exploring the workspaces module trying to switch multiple workspaces I saw that below the 768px screen when we click on any workspace the modal content text is not completely visible it is hidden because some inline CSS is coming which is - max-height: 0px;

Steps to reproduce

  1. Enable the workspace module.
  2. Switch the screen below 768px
  3. Click on workspace to the right side.
  4. See the modal content text.

Proposed resolution

Unset the max-height - max-height: unset;

https://git.drupalcode.org/project/drupal/-/merge_requests/6019/

Remaining tasks

Commit

User interface changes

Before

before

After

after

after

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#21 Screenshot 2024-02-28 at 9.30.13 AM.png825.87 KBgauravvvv
#17 AfterFix2.png166.82 KBdivya.sejekan
#17 AfterFix1.png151.31 KBdivya.sejekan
#17 BeforeFix.png119.19 KBdivya.sejekan
#13 Beforepatchtext.png31.41 KBkanchan bhogade
#13 Afterpatchtext.png40.37 KBkanchan bhogade
#10 Screenshot at Jan 04 15-42-15.png175.43 KBshweta__sharma
#8 Patch Error.png349.11 KBsandeep_k
#8 Before Patch.png107.55 KBsandeep_k
Screenshot at Dec 19 13-36-36.png162.78 KBshweta__sharma
screen-capture19Dec.mp46.05 MBshweta__sharma

Issue fork drupal-3409640

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

shweta__sharma created an issue. See original summary.

samir_shukla’s picture

samir_shukla
Primary language English
Location bareilly
commented
Assigned: Unassigned » samir_shukla

Hi, working on the issue.

samir_shukla’s picture

samir_shukla
Primary language English
Location bareilly
commented
Assigned: samir_shukla » Unassigned

Meeni_Dhobale made their first commit to this issue’s fork.

swatidhurandhar made their first commit to this issue’s fork.

swatidhurandhar opened merge request !6019

swatidhurandhar’s picture

swatidhurandhar commented
Status: Active » Needs review

I have created a MR where I have set the max-height of UI dialog content for workspace module.

sandeep_k’s picture

sandeep_k commented
StatusFileSize
new Before Patch.png107.55 KB
new Patch Error.png349.11 KB

@swatidhurandhar I have tested the shared patch MR !6019 mergeable I was able to reproduce this issue but was unable to apply the shared patch. Sharing the error here which I was getting while applying the patch.

swatidhurandhar’s picture

swatidhurandhar commented

@Sandeep_k It is warning not error, the patch is adding the code.

shweta__sharma’s picture

shweta__sharma commented
StatusFileSize
new Screenshot at Jan 04 15-42-15.png175.43 KB

Hi @swatidhurandhar

I tested your Merge request! 6019 and it fixed the issue but there are some warnings you need to check. I think your editor using tab space.

core/6019.patch.txt:36: tab in indent.
		.ui-dialog {
core/6019.patch.txt:37: tab in indent.
			.ui-dialog-content {
core/6019.patch.txt:38: tab in indent.
				max-height: 5rem !important;
core/6019.patch.txt:39: tab in indent.
			}
core/6019.patch.txt:40: tab in indent.
		}
Checking patch core/modules/workspaces/css/workspaces.off-canvas.css...
Checking patch core/modules/workspaces/css/workspaces.off-canvas.pcss.css...
Checking patch core/modules/workspaces/css/workspaces.off-canvas.pcss.css...
Checking patch core/modules/workspaces/css/workspaces.off-canvas.pcss.css...
Applied patch core/modules/workspaces/css/workspaces.off-canvas.css cleanly.
Applied patch core/modules/workspaces/css/workspaces.off-canvas.pcss.css cleanly.
Applied patch core/modules/workspaces/css/workspaces.off-canvas.pcss.css cleanly.
Applied patch core/modules/workspaces/css/workspaces.off-canvas.pcss.css cleanly.
warning: squelched 2 whitespace errors
warning: 7 lines add whitespace errors.
sandeep_k’s picture

sandeep_k commented

Thanks @swatidhurandhar, I have checked & the code is updated.
Testing Steps-

  • Enable the workspace module.
  • Download & apply the patch.
  • Switch the screen below 768px
  • Click on workspace on the right side & change the workspace.
  • Reverify the modal content text.

Testing Results
Error is fixed successfully after applying the patch, We can remove the space to fix these warnings as well. Sharing after results. RTBC ++

shweta__sharma’s picture

shweta__sharma commented

Added code inside the media query as above 767px screen the modal content is working fine so I added media query below 767px
Thanks

kanchan bhogade’s picture

kanchan bhogade commented
StatusFileSize
new Afterpatchtext.png40.37 KB
new Beforepatchtext.png31.41 KB

Hi,
I verified and tested MR!6019 on Drupal version- 11.0-dev.
The patch was applied successfully.

Testing Steps:

  1. Set up Drupal 11.
  2. Enable the workspace module.
  3. Switch the screen below 768px
  4. Click on workspace on the right side.
  5. See the modal content text, which gets cropped
  6. Download the patch and Apply.
  7. Reverify the modal content text for the screen below the 768px.

Testing Results:
Now the modal content text is not cropped and it's fully visible
screenshots added for reference

Moving to RTBC++

Gauravvvv made their first commit to this issue’s fork.

gauravvvv’s picture

gauravvvv commented

Instead of hardcoding a value, we can simply unset the max-height added by JavaScript to resolve the height issue.

I have updated the MR.

divya.sejekan’s picture

divya.sejekan commented
Assigned: Unassigned » divya.sejekan
divya.sejekan’s picture

divya.sejekan commented
Assigned: divya.sejekan » Unassigned
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new BeforeFix.png119.19 KB
new AfterFix1.png151.31 KB
new AfterFix2.png166.82 KB

Verified the Issue after the update in the MR . Issue is fixed

Testing Steps:
1. Enable the workspace module.
2. Switch the screen below 768px
3. Click on workspace on the right side.
4. See the modal content text, which gets cropped
5. Download the patch and Apply.
6. Reverify the modal content text for the screen below the 768px.

Testing Results:
The modal content text is not cropped and it's fully visible on or below 768px
screenshots added for reference

Moving to RTBC++

quietone’s picture

quietone commented

I'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions or other work to do.

@shweta__sharma, thanks for reporting this problem.

Since this change the UI it should have the latest before and after screenshots linked to in the Issue Summary. That way reviewers and committers can easily find them. The proposed resolution is to remove max-height: 0px; but that line is not removed in the MR. For this short issue I don't think those points are enough to warrant sending this back to needs work. However, other committers may choose to do so.

It really is good practice in Drupal to help reviewers and committers to have the Issue Summary up to date. A complete issue summary is inviting.

shweta__sharma’s picture

shweta__sharma commented
Issue summary: View changes

Sure @quietone

Updated IS template.

Thanks

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

I'd very much rather not having to use !important.

Can we avoid this? making the rule more specific could work.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new Screenshot 2024-02-28 at 9.30.13 AM.png825.87 KB

We're overriding the value of max-height added by JavaScript using !important.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Reviewed & tested by the community

works for me, thanks for the context

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

Can we add a comment explaining that in the CSS? It'll help future us find out why !important was used.

Mithun S made their first commit to this issue’s fork.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Reviewed & tested by the community

Restoring status, RTBC as a comment has been added to CSS code.

  • nod_ committed 00329c22 on 11.x 
    Issue #3409640 by swatidhurandhar, Mithun S, shweta__sharma, Gauravvvv,...

  • nod_ committed 33ec7705 on 10.3.x 
    Issue #3409640 by swatidhurandhar, Mithun S, shweta__sharma, Gauravvvv,...

  • nod_ committed a55f5739 on 10.2.x 
    Issue #3409640 by swatidhurandhar, Mithun S, shweta__sharma, Gauravvvv,...

nod_ closed merge request !6019

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 00329c2 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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