Problem/Motivation
Follow-up to #2905222: Migrate UI: Initial Screen UX improvements. @xjm made some review comments that will be addressed here:
+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php @@ -142,16 +142,26 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) { - '#markup' => '<p>' . $this->t('Upgrade a site by importing it into a clean and empty new install of Drupal 8. You will lose any existing configuration once you import your site into it. See the <a href=":url">online documentation for Drupal site upgrades</a> for more detailed information.', [ + '#markup' => '<p>' . $this->t('Upgrade a site by importing its database and files into a clean and empty new install of Drupal 8. See the <a href=":url">Drupal site upgrades handbook</a> for more information.', [
We're not importing the site's database and files. We're importing the data from its database and files.
+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php @@ -142,16 +142,26 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) { + $legend[] = $this->t('<em>Old site:</em> the site you want to upgrade.'); + $legend[] = $this->t('<em>New site:</em> this empty Drupal 8 installation you will import the old site to.'); + + $form['legend'] = [ + '#theme' => 'item_list', + '#title' => $this->t('Definitions'), + '#list_type' => 'ul', + '#items' => $legend, + ];
If it's a list of definitions, should it use a definition list?
+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php @@ -142,16 +142,26 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) { + $info[] = $this->t('You may need multiple tries for a successful upgrade so <strong>backup the database</strong> for this new site. The upgrade will change it and you may want to revert to its initial state.');
"Back up" needs to be two words when it's a verb. (It's correct in HEAD.)
Also "you may need multiple tries" is kind of confusing ("a try" as a noun might not be a familiar expression); "attempts" might be better. But reading this I might wonder why I might need multiple attempts; it's not like shooting a basketball.
Finally, we aren't using the defined term "new site" here, and if I'm starting from a "clean and empty new install" then why would I want to back up the database of that?
+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php @@ -160,12 +170,13 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) { + '#title' => $this->t('Steps to prepare for the upgrade'),
How about "Preparation steps"?
Proposed resolution
Address comments.
Remaining tasks
Make changes.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#16 | 2908699-16.patch | 6.18 KB | jofitz |
#16 | interdiff-14-16.txt | 3.18 KB | jofitz |
#14 | 2908699-14.patch | 6.17 KB | jofitz |
#14 | interdiff-11-14.txt | 1023 bytes | jofitz |
#11 | 2908699-11.patch | 6.16 KB | jofitz |
Comments
Comment #2
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #3
jofitz CreditAttribution: jofitz at ComputerMinds commentedI have made a start on the changes, but need some help/clarification about a couple of things. There's no need to set the status to "Needs review" and trouble the testbot because this is still a work in progress.
Todo:
2. Make
$form['legend']
into a definition list.3. Re-write the sentence about backing up the new site.
Comment #4
neerajsinghOn item number 3, Yes I agree to @xjm.
We need not backup database as this would be a new installation of Drupal8.
In-fact if we restore the database and try again to perform the migrate, it migrates only the new changes(between the previous migration and the next attempt) in the legacy(Drupal6/7) system and not the complete application.
Step to re-generate the scenario in the above sentence:
Hence, I would suggest to re-install Drupal8 setup in this case, instead of re-storing the backup database.
Comment #5
heddnYou may want to run multiple tries for a successful upgrade so <strong>backup the database</strong> for this new site. The upgrade will change it and you may want to revert to its initial state.
I think this was more important when incremental migrations were a thing via the UI. Since it isn't a thing right now, adding special instructions to back up is pointless and something I personally have never done.
So, if we can agree on removing the line, all that is left is a definition list.
Comment #6
heddnComment #7
heddnShould this get postponed on #54898: Add a description-list.html.twig template (ex. definition list)? Or just hack up the markup without a theme layer? Or just say good enough is good enough?
Comment #8
xjmI don't think we need to postpone this. We could either hardcode the markup in the render array, or just to create a specific template. There are a couple examples in core. If you grep for
<dl>
most are inhook_help()
which is not currently themeable and just concatenated and then converted to markup (so not really applicable yet as this isn't ahook_help()
), but there are also examples like these:core/themes/stable/templates/admin/block-content-add-list.html.twig
Comment #9
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #11
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #12
quietone CreditAttribution: quietone as a volunteer commentedThis looks good, thanks everyone. Just one small nit.
importing the data from its database and files into
This could be read as importing the data from the files and not importing the files. I know it is just a quirk but perhaps it should be explicit that the fiies are also imported?
Comment #13
xjmThis is just some improvement for the UI text, but it doesn't need to block the stable release.
Comment #14
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdjusted the sentence structure to avoid confusion.
Comment #16
jofitz CreditAttribution: jofitz at ComputerMinds commentedForgot to update the tests too!
Comment #17
heddnThis is now good to go. The feedback from the IS is all addressed and all the nits responded to.
Comment #19
Gábor HojtsyComment #20
Gábor HojtsySuperb, thanks all!