Overall I think these directions are good (although I'm not a copy writer). Two suggestions:

  • Most people tend to scan vs read, so having a list is great. As is the bold key text on point #1. Recommend bolding the key point of each bullet point.
  • Currently the Migrate is a one-shot deal, you can't redo it, I think this should be stressed here, maybe an additional point or added to the last sentence (in bold).

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jjpoole created an issue. See original summary.

yoroy’s picture

Just clarifying things a bit. Thanks for reviewing and starting the issue.

heddn’s picture

Issue tags: +Migrate UI, +Migrate critical

Tagging

heddn’s picture

yoroy’s picture

Status: Active » Needs review
  1. I tried to find *parts* for each bullit to emphasize but some would need the whole sentence to be bolded so maybe don't emphasize anything?
  2. I incorporated the one-shot deal thing into the first bullit, does that work?
  3. Added a old site/new site explainer to make the actual bullits a bit easier to read.

---

Upgrade a site by importing its database and files into a clean and empty new install of Drupal 8. See the [Drupal site upgrades handbook] for more information.

  • Old site: the site you want to upgrade
  • New site: this empty Drupal 8 installation you will import the old site into.
  1. You may need multiple tries for a succesful upgrade so backup the database for this new site. The upgrade will change it and you may want to revert to its initial state.
  2. Make sure that the database for the old site is accessible on the host of this new site.
  3. If the old site has private files, a copy of its files directory must also be accessible on the host of this new site.
  4. Enable all modules on this new site that are enabled on the old site. For example, if the old site uses the book module, then enable the book module on this new site so that the existing data can be imported into it.
  5. Do not add any content to this site before upgrading. Any existing content is likely to be overwritten by the upgrade process. See [the upgrade preparation guide].
  6. Put this site into [maintenance mode].

The upgrade can take a long time. It is better to upgrade from a local copy of your site instead of directly from your live site.

quietone’s picture

I quite like the old site/new site definitions.

One question is the about the phrases 'import the old site into' and 'imported into it'. Is that right? I mean I would just say, 'import the old site to' and 'imported to it'. I don't mind at all what is used, I am just curious.

yoroy’s picture

Thanks! Me no native speaker so by all means correct or simplify where necessary :)

quietone’s picture

And I am old enough that my native language has been changed by the young ones (as they should). I am not sure what is correct, either!

edysmp’s picture

edysmp’s picture

heddn’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -142,16 +142,25 @@ class MigrateUpgradeForm extends ConfirmFormBase {
    +      $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.');
    

    Let's add a title/label of definitions to these.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -142,16 +142,25 @@ class MigrateUpgradeForm extends ConfirmFormBase {
    +      $info[] = $this->t('You may need multiple tries for a succesful upgrade so backup the database for this new site. The upgrade will change it and you may want to revert to its initial state.');
    

    Let's bold 'backup the database'

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -142,16 +142,25 @@ class MigrateUpgradeForm extends ConfirmFormBase {
    +      $info[] = $this->t('Make sure that the database for the old site is accessible on the host of this new site.');
    

    Let's change that to "Make sure that <strong>access to the database</strong> for the old site is available from this new site."

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -142,16 +142,25 @@ class MigrateUpgradeForm extends ConfirmFormBase {
    +      $info[] = $this->t('If the old site has private files, a copy of its files directory must also be accessible on the host of this new site.');
    

    Bold, "If the old site has private files"

  5. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -142,16 +142,25 @@ class MigrateUpgradeForm extends ConfirmFormBase {
    +      $info[] = $this->t('Enable all modules on this new site that are enabled on the old site. For example, if the old site uses the book module, then enable the book module on this new site so that the existing data can be imported to it.');
    

    Bold, "Enable all modules on this new site"

  6. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -142,16 +142,25 @@ class MigrateUpgradeForm extends ConfirmFormBase {
    +      $info[] = $this->t('Do not add any content to this site before upgrading. Any existing content is likely to be overwritten by the upgrade process. See <a href=":url">the upgrade preparation guide</a>.', [
    

    Bold: "Do not add any content to the new site".
    And let's use the term from our definition and call it new site.

yoroy’s picture

Agreed with all those changes @heddn, you found the right parts to make bold after all :)

heddn’s picture

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -142,16 +142,25 @@ class MigrateUpgradeForm extends ConfirmFormBase {
+      $legend[] = $this->t('<em>New site:</em> this empty Drupal 8 installation you will import the old site to.');

I just now reviewed #6 a little more closely. Isn't there something we were taught about dangling prepositions? I seem to recall something about that. How's this to improve things?

<em>New site:</em> this empty Drupal 8 installation into which you will import the old site.

edysmp’s picture

Assigned: Unassigned » edysmp

working in this.

quietone’s picture

edysmp’s picture

Assigned: edysmp » Unassigned
Status: Needs work » Needs review
FileSize
4 KB
2.93 KB

1. I don't know what label to use.

All others point fixed.

edysmp’s picture

Status: Needs review » Needs work

The last submitted patch, 16: usability-upgrade-initial-screen-2905222-16.patch, failed testing. View results

quietone’s picture

The bolding looks good to me. Reading down, every line has some emphasis, and it even built up the expectation that each line would. And then point 6 has none, and I was surprised. But I guess it makes sense since the link is the important bit.

heddn’s picture

Reviewed in weekly migrate meeting and agreed this is in fact a migrate critical.

heddn’s picture

Title: Upgrade: Initial Screen » Migrate UI: Initial Screen UX improvements
heddn’s picture

Updated screenshot with an attempt at adding a label to these things.

Status: Needs review » Needs work

The last submitted patch, 22: 2905222-22.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
8.11 KB
3.82 KB

Fixing tests.

Status: Needs review » Needs work

The last submitted patch, 24: 2905222-24.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
7.88 KB
3.29 KB
quietone’s picture

Status: Needs review » Needs work

Looking good. Just one reservation.

+++ 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>New site:</em> this empty Drupal 8 installation you will import the old site.');

'you will import the old site.' To where? Still go the for preposition at the end 'to'. So it becomes, 'you will import the old site to.' Far better than the odd construction 'into which you will import the old site.'

Maybe we need some else to offer their opinion?

quietone’s picture

This is what I am thinking. To keep RTBC, I'll resist making the change myself.

+++ 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>New site:</em> this empty Drupal 8 installation you will import the old site.');

s/site/site to./

heddn’s picture

Status: Needs work » Needs review
FileSize
7.88 KB
797 bytes
quietone’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

With such a small change, I'll RTBC this while I am here.

larowlan’s picture

  1. +++ 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');
    

    nit: there are two spaces here - can be fixed on commit

  2. +++ 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 succesful 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.');
    

    nit: successful (two s's) - same - can be fixed on commit

yoroy’s picture

Status: Reviewed & tested by the community » Needs work
  1. Another nit: one of the definitions ends with a period. The other does not
  2. "Important notes" is almost meaningless. Everything on a screen is important or we wouldn't show it :) Can we do something like "Steps to prepare for the upgrade." or "Take these steps before upgrading" or "How to prepare for the upgrade"?
Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
7.9 KB
2.18 KB

Corrected nits highlighted in #31 and #32.

Jo Fitzgerald’s picture

Status: Needs review » Reviewed & tested by the community

Corrections don't make any functional difference so returning to RTBC.

yoroy’s picture

Agreed. Thanks for the quick turnaround @Jo Fitzgerald!

  • larowlan committed 6cbb549 on 8.5.x
    Issue #2905222 by heddn, edysmp, Jo Fitzgerald, quietone, yoroy, jjpoole...

  • larowlan committed 9ffe00f on 8.4.x
    Issue #2905222 by heddn, edysmp, Jo Fitzgerald, quietone, yoroy, jjpoole...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 6cbb549 and pushed to 8.5.x

Cherry-picked as 9ffe00f and pushed to 8.4.x

xjm’s picture

Nice work on this. A few points:

  1. +++ 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.

  2. +++ 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?

  3. +++ 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?

  4. +++ 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"?

xjm’s picture

Sorry, I totally opened this tab this morning and didn't see that @larowlan committed it hours ago before I reviewed. Things in #39 can be a followup if preferred.

Jo Fitzgerald’s picture

Status: Fixed » Closed (fixed)

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