In #1821548: Add a "diff" of some kind to the CMI UI, we're opening a dialog with a hard-coded width of 700px. That's not very mobile friendly. Instead of doing that, let's define some dialog classes (e.g., small, medium, large) and figure out good CSS for them. Any volunteers?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Issue tags: +dialog

and another tag

quicksketch’s picture

Issue tags: -dialog

I'm not sure this should happen. Instead each modal should be given a specific class and all the sizing should be done via CSS (and media queries within that CSS). #1851414: Convert Views to use the abstracted dialog modal introduces a dialog.theme.css file, I think it would make sense to simply specify a default width in there and try to avoid any manual setting of width at all.

quicksketch’s picture

Issue tags: +dialog
dealancer’s picture

Right now default width is 300px set by jQueryUI. However it would be nice to have different default widths for different screen widths/devices (e.g. mobile, desktop). This should be done using @media in css.

dealancer’s picture

Ok, so here are couple of concerns when passing width directly into dialog:

A. It is hard to adopt such dialog to the smaller screen.
B. Dialog is not centered when screen is resized.

So I almost agree with @effulgentsia re introducing classes, but I also like idea of @quicksketch to use default width and be able to override it. Also I want to make dialog responsive. Here is a patch that can do all of this.

Usage examples:

    $response->addCommand(new OpenModalDialogCommand($title, $dialog_content)); // 700 px, responsive;
    $response->addCommand(new OpenModalDialogCommand($title, $dialog_content, array('dialogClass' => 'wide'))); // 940 px, responsive
    $response->addCommand(new OpenModalDialogCommand($title, $dialog_content, array('dialogClass' => 'narrow'))); // 460 px, responsive
    $response->addCommand(new OpenModalDialogCommand($title, $dialog_content, array('width' => '555px'))); // overriden
dealancer’s picture

Status: Active » Needs review

Status: Needs review » Needs work
Issue tags: -mobile, -dialog

The last submitted patch, dialog-width-responsive-1918744-5.patch, failed testing.

rteijeiro’s picture

Status: Needs work » Needs review
Issue tags: +mobile, +dialog
dealancer’s picture

Ok, looks like we need to fix a test.

Here is re-rolled patch!

dealancer’s picture

Last one was empty.

This patch contains updated test.

oresh’s picture

Status: Needs review » Needs work

Everything looks ok,
Can you please just change px to % ?

effulgentsia’s picture

I'm not a front-end developer, so don't have an opinion on #11, but otherwise, I think #10 looks good. I'm curious what @quicksketch thinks though, given #2.

dealancer’s picture

Status: Needs work » Needs review

Re #11, also can't find use cases when we need this option.

We would rather have strictly defined width so the appearance of dialog box is predictable. When it starches to much the content can "swim" to the borders of the monitor. Using media queries we can easily control width of the dialog box for smaller device.

We would like to have more reviews and opinions. So I am settings status back to needs review, otherwise this ticket can be ignored.

valdo’s picture

Status: Needs review » Needs work

The patch does not apply to me. When trying to apply I get these errors:

error: core/modules/config/config.admin.inc: No such file or directory
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Ajax/DialogTest.php:37
error: core/modules/system/lib/Drupal/system/Tests/Ajax/DialogTest.php: patch does not apply
error: patch failed: core/modules/system/system.module:1317
error: core/modules/system/system.module: patch does not apply

Looks as in the meantime filesystem as changed, core/modules/config/config.admin.inc doesn't exist anymore.

LewisNyman’s picture

I think we should be writing the widths in percentages, all our core CSS is.

Looking at the initial problem, it seems that we only need one fluid size in order to make it mobile friendly. Why are we adding different sizes?

quicksketch’s picture

Status: Needs work » Closed (won't fix)

FYI in Editor module we just used CSS ultimately:

/**
 * @file
 * Styles for text editors.
 */
.editor-dialog {
  /* This !important is required to override inline CSS of jQuery UI. */
  width: 80% !important;
  max-width: 500px;
}

@media screen and (max-width: 600px) {
  .editor-dialog {
    width: 95% !important;
  }

The unfortunate thing however is that jQuery UI loads FIRST on pages where Edit module is present, since it depends on jQuery UI at page load time and editor.css is loaded on-the-fly after the page load. So we ended up with !important in there until we fix that issue or (better yet) just stop loading the default CSS for jQuery UI (which we override everywhere anyway already).

So I think this is a won't fix. Just use CSS to set dialog widths.

quicksketch’s picture

Issue summary: View changes

.