Improve the tour edit page

uhh damn when i reread the issue summary after i saw the notification that you assigned yourself i realized i'Ve created a dedicated issue 🐛 The page title "edit tour" is too generic Needs review for point 1 in the proposed resolution. there were too many issues i've created and i completely lost track and forgot that about the linked issues. :( that was not intended.

No worries still got 2 +3 to do here.

ah you already updated the issue summary, thanks! and one thought i had comparing the tour with for example views, would it be useful to also add a description field to a tour? for the sake of consistency it would make sense but probably also it could be helpful to provide the gist what a tour is about?

On save redirect the user back to the tour list page, that way it would be inline with the behavior of for example content types and alike

Actually disagree with this one.

If did it a user would have to
1. Create tour
2. Click back into tour
3. Click into tips

If you're creating a tour you're going to probably immediately create tips so going back to the list page doesn't make sense.

If anything it should redirect to the tips form.

ahhh it bit me again that i've assumed that changes would be persistent across tabs/local items and then saved on pressing the save button across tabs. that assumption slips in from time to time. so yeah you are totally right making a redirect would be the wrong call. but it still leaves the problem that there is no direct way in the form to go back one step to the tour list view. you have to use the breadcrumbs at the moment or the navigation. but it is still advisable to have another option to move forward. that is a pattern we tried to follow in the ux meetings. one option might be having a second button (or better a third button aside save and delete) like used on the add term page: admin/structure/taxonomy/manage/tags/add

the other changes introducing the local items look great!

I’m actually pretty against adding a back to list button on the tour edit form. That’s what the breadcrumbs are for and feature isn’t needed for views or themes.

But on the new tips form I’d be open to add another button to keep adding tips. That workflow makes sense

So

New tour = edit form
Edit tour = edit form
Add tip (save) = new tip form
Add tip (save return to list) = tip list form

Wanted to test something before i answer to your point. But i ran into an error which only happens with the feature branch checked out. on 2.0.x i am able to add a new text tip, while i am on the feature branch and on the local item for tips and add a new text tip i run into:

The website encountered an unexpected error. Try again later.

TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given, called in /var/www/html/repos/drupal/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238 in Drupal\Component\Utility\Html::escape() (line 431 of core/lib/Drupal/Component/Utility/Html.php).
Drupal\Component\Render\FormattableMarkup::placeholderEscape() (Line: 211)
Drupal\Component\Render\FormattableMarkup::placeholderFormat() (Line: 195)
Drupal\Core\StringTranslation\TranslatableMarkup->render() (Line: 15)
Drupal\Core\StringTranslation\TranslatableMarkup->__toString() (Line: 54)
Drupal\Core\Messenger\Messenger->addMessage() (Line: 287)
Drupal\tour\Form\TourTipsListForm->submitForm() (Line: 185)
Drupal\tour\Form\TourTipsListForm->tipAdd()
call_user_func_array() (Line: 105)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 43)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 589)
Drupal\Core\Form\FormBuilder->processForm() (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult() (Line: 39)
Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult() (Line: 80)
Drupal\workspaces\Controller\WorkspacesHtmlEntityFormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 709)
Drupal\Core\DrupalKernel->handle() (Line: 19)

To add the new buttons

As soon as I started to work on it I talked myself out of the "save return to list" for tips too. The tipList form is where you select the tip type and that determines the form that displays when adding a tip.

Changing all that around doesn't seem worth it. So

New tour = edit form
Edit tour = edit form
Add tip = tip list form
Edit tip = tip list form.

hm when i am trying to add a tip i am still running into a wsod ( was trying to add a tip to the appearance page). the error seems to be the same:

The website encountered an unexpected error. Try again later.

TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given, called in /var/www/html/repos/drupal/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238 in Drupal\Component\Utility\Html::escape() (line 431 of core/lib/Drupal/Component/Utility/Html.php).
Drupal\Component\Render\FormattableMarkup::placeholderEscape() (Line: 211)
Drupal\Component\Render\FormattableMarkup::placeholderFormat() (Line: 195)
Drupal\Core\StringTranslation\TranslatableMarkup->render() (Line: 15)
Drupal\Core\StringTranslation\TranslatableMarkup->__toString() (Line: 54)
Drupal\Core\Messenger\Messenger->addMessage() (Line: 287)
Drupal\tour\Form\TourTipsListForm->submitForm() (Line: 185)
Drupal\tour\Form\TourTipsListForm->tipAdd()
call_user_func_array() (Line: 105)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 43)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 589)
Drupal\Core\Form\FormBuilder->processForm() (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 709)
Drupal\Core\DrupalKernel->handle() (Line: 19) 

that i have to admit i not completely understand what you mean?

New tour = edit form
Edit tour = edit form
Add tip = tip list form
Edit tip = tip list form.

but in general. i agree maybe dont add the return to the tour list page at this point and commit the issue when the wsod is fixed. i see your point but at the same time i dont think relying only onto the breadcrumb is the right choice either. that has flaws, in particular for keyboard users in regards of convenience but i have to think about the problem more thoroughly and compare a few more pages in the admin ui. that shouldnt delay this issue. but i am setting it back to needs work because of the error on add tip.

I think that might be your local as I'm able to create tours and tips and the tests are doing the same.

hm odd. i have a fresh instance and core as well as tour are each based on the git repo and the feature branch is checked out plus caches cleared. so i am not sure what might be causing the problem on my end locally? or what might be different on my end?!

Still can’t trigger the error

But have a test failure for next major need to resolve

Fixed the test and issue seen on 11.x

I can confirm that the error is gone. i am able to add a tip now. and based on your changes here https://git.drupalcode.org/project/tour/-/merge_requests/49/diffs?commit... it validates my assumption that the error was caused by an empty label when a tip is being added.

But being able to actually test the behavior now i wonder if

 if (!empty($input_values) && array_key_exists('new', $input_values)) {
      $this->messenger()->addMessage($this->t('The tour tips has been updated.'));
    }
    else {
      $this->messenger()->addMessage($this->t('The tour tips has been created.'));
    }

is necessary at all. at the moment when i add a new tour tip i get "The tour tips has been updated" as the status message and i see an empty to be populated "add tip" form. strictly speaking nothing happened here yet, neither an update nor the addition of a tip. on the other hand when i edit an existing tour tip i don't get a status message at all. i would expected to get the other message of the two in the if/else condition. so probably the conditions in the if clause are not correct? but i still wonder if that if/else condition is necessary at all in the first place since the status is more confusing and ambiguous than actually helping?

and on a side note "tip" should be in the singular form not plural otherwise it would have to be "have" instead of "has"?

Alternative is no message at all and definitely a -1 for that.

but what is the purpose and benefit of these status messages when the user arrives on the add tour tip page or an existing tour tip page, where no addition of any content, nor any edit of any content, nor any save of the form (add/edit form) has happened yet?

quickly recorded a video illustrating the current behavior

They only appear when the form saves.

no, as i've illustrated in the video. i just made a quick change:

    if (!empty($input_values) && array_key_exists('new', $input_values)) {
      $this->messenger()->addMessage($this->t('The tour tips has been updated. UPDATED'));
    }
    else {
      $this->messenger()->addMessage($this->t('The tour tips has been created.'));
    }

i just added an UPDATED to the returned string for the if clause. and as you can see the status message shows on the add tip form BEFORE the form is saved. plus it would have to be messaage form the else statement that the tip has been created. but still the messages are shown before the form was saved.

Will take a look but definitely leaving some message

yeah no rush but as i've mentioned on the thread on slack. just comment out line 286-291 and then try to save the form. you still get status message about a tip being created or being update. so the messages are in place also without that if/else statement, plus they are more specific because they also contain the title of the tour tip that is being added or updated. i am not against those status message in any way, that feedback is important. but that if/else is adding some additional and redundant messages before anything happened is my point.

Should be fixed

looks good now. went through all steps outlined before and i've also cross checked the settings page for the tour module saving the settings i havent checked before. looks all good , no errors and every saving of the form state has still a status message. thanks!

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