Use styling from Upgrade Status module on Review form

That's quite a nice improvement!

We discussed this issue briefly in the Usability meeting. Mostly we were talking about #3024682: Migrate UI - add human-friendly module names to the Review form.

I am updating the issue summary with the most recent screenshot from that issue.

working on it.

Reroll for 9.1.x-dev and solving coding standard error also.

Hi @quieton, Hope you fine!!
no issues, you can continue working on this issue.

Drupal\KernelTests\Core\Theme\Stable9LibraryOverrideTest is failing because the patch is adding a new library without having a copy of that in Stable. I'm wondering if some of the CSS should be added in Seven / Claro since the screenshots are only showing Seven. If we want to keep the CSS in the module, we should add a copy of the file to Stable which should make the test pass.

Fixed test, Please review.

I will review the changes to ReviewForm.php later, but for now I have a few requests that will simplify this patch.

1. We do not need to add copies of the SVG files. The patch already includes this in a context line, referring to the existing copies:

    +++ b/core/modules/migrate_drupal_ui/css/components/upgrade-analysis-report-tables.css
    @@ -21,3 +21,13 @@
     .upgrade-analysis-report__status-icon--error:before {
       background-image: url(../../../../misc/icons/e32700/error.svg);
     }

   We should do the same in the new CSS files instead of this:

    +++ b/core/modules/migrate_drupal_ui/css/migrate_drupal_ui.admin.theme.css
    @@ -0,0 +1,219 @@
    ...
    +.migrate-drupal-ui-summary tr.known-errors > td.status-info {
    +  background-image: url(../icons/error.svg);
    +}

2. Do we really need a 219-line CSS file to style this table? Can’t we reuse some existing CSS rules?

3.   +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
     @@ -225,10 +225,10 @@ public static function run($initial_ids, $config, &$context) {
              $migration = \Drupal::service('plugin.manager.migration')->createInstance($migration_id);
              $migration_name = $migration->label() ? $migration->label() : $migration_id;
              $context['message'] = (string) new TranslatableMarkup('Currently upgrading @migration (@current of @max total tasks)', [
     -            '@migration' => $migration_name,
     -            '@current' => $context['sandbox']['current'],
     -            '@max' => $context['sandbox']['max'],
     -          ]) . "<br />\n" . $context['message'];
     +          '@migration' => $migration_name,
     +          '@current' => $context['sandbox']['current'],
     +          '@max' => $context['sandbox']['max'],
     +        ]) . "<br />\n" . $context['message'];
            }
          }
          else {

   The indentation should be fixed, but not as part of this issue. In other words, this change is out of scope.

4. Same comment here:

    +++ b/core/modules/migrate_drupal_ui/src/Form/OverviewForm.php
    @@ -35,9 +35,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   
         $form['info_header'] = [
           '#markup' => '<p>' . $this->t('Upgrade a site by importing its files and the data from its database into a clean and empty new install of Drupal @version. See the <a href=":url">Drupal site upgrades handbook</a> for more information.', [
    -          '@version' => $this->destinationSiteVersion,
    -          ':url' => 'https://www.drupal.org/upgrade/migrate',
    -        ]),
    +        '@version' => $this->destinationSiteVersion,
    +        ':url' => 'https://www.drupal.org/upgrade/migrate',
    +      ]),
         ];
   
         $form['legend']['#markup'] = '';

5. Same comment here. All this does is fix a missing EOL.

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/files/core/tests/fixtures/files/html-1.txt
    @@ -1 +1 @@
    -<h1>SimpleTest HTML</h1>
    \ No newline at end of file
    +<h1>SimpleTest HTML</h1>

6. Out of scope:

    +++ b/core/modules/migrate_drupal_ui/tests/src/Kernel/MigrationLabelExistTest.php
    @@ -28,7 +28,7 @@ public function testLabelExist() {
   
         /** @var \Drupal\migrate\Plugin\MigrationPluginManager $plugin_manager */
         $plugin_manager = $this->container->get('plugin.manager.migration');
    -    // Get all the migrations
    +    // Get all the migrations.
         $migrations = $plugin_manager->createInstances(array_keys($plugin_manager->getDefinitions()));
         /** @var \Drupal\migrate\Plugin\Migration $migration */
         foreach ($migrations as $migration) {

Now that #3024682: Migrate UI - add human-friendly module names to the Review form has been fixed, we do not need to include caveats about it, so I am updating the issue summary. The question about two or three columns is also obsolete.

@quietone:

Thanks for addressing the points I made in #31. That simplifies the patch quite a bit.

I think there are several things we need to do before this issue can go in, so I am setting the status back to NW. I will try to start with the most important points.

1. There are still some things undone in the Remaining tasks section of the issue summary. (There are also some things that have been done, and I am updating that section.) In particular, the filtering has not been implemented. In the summary row at the top (X modules will not upgrade … Y modules will upgrade) there are "Details" links that do not do anything.
2. Possibly related to the first item: I am worried about the accessibility of having to search through long tables for the few "Will not upgrade" among the many "Will upgrade" (or vice versa). Personally, I can easily scan the table thanks to the color coding, but someone who uses a screen reader or is red/green color blind will have more trouble. If the proposed solution is a step backwards for some users from the current two tables (one table each for will/will not be upgraded) then we cannot accept it.
3. As the Remaining tasks list points out, we need some input from a designer. Personally, I do not like the way the table is embedded in the details element, with two gray headers and a little bit of white space at the edges.
4. Is the Upgrade status page (/admin/reports/updates/update) the best model to follow? It does not have any details elements (a.k.a. collapsible fieldsets). Maybe a better model is the Extend page (/admin/modules). That already has a table inside a details element. The main differences are that the header row of the Extend page table is visually hidden and that the column widths are closer to equal on the review form. (On the Extend page, the first column is so narrow, containing just a checkbox, that you might call it a two-column table.)
5. Now that #3024682: Migrate UI - add human-friendly module names to the Review form has been fixed, we have an extra column: the machine name of the D6/D7 module.
6. We should not hard-code the version number (Drupal 8) in the table header.
7. With all of that horizontal space available, I would consider making the detail summary (e.g., "Core: 2 of 32 will not upgrade") a single line instead of two.
8. Take into account the simplifications discussed in #3134300: Simplify ReviewForm::buildForm() and the resulting markup. In the current patch on this issue, we already have CSS classes on the rows, which was one of my suggestions on that issue. I think the additional classes on the cells in the Status row (and the extra span used to add those classes) are now obsolete.

We discussed this issue at the #3156568: Drupal Usability Meeting 2020-07-07. (It may be a while before a recording of the meeting is posted to that issue.)

Others at that meeting confirmed my concern in #34.2. That is, the existing organization into two tables (modules that will be updated and modules that will not be updated) is more accessible/usable than the reorganization in the current patch.

FWIW, the contrib Upgrade Status module, which we are using as a model for this issue, is planning to change the way it presents its information. See #3153862: [META] Improve user interface, so it drives what people need to do rather than merely display data.

In short, I think that we should narrow the scope of this issue. Make the tables prettier, but do not reorganize them.

Also, pay attention to the changes in #3134300: Simplify ReviewForm::buildForm() and the resulting markup, which is already RTBC. I am adding that as a related issue. Perhaps, as part of this issue, we can implement the suggestion I made there to move the CSS classes from a table cell to the table row.