It's fantastic usability that add a new term comes up with the form for adding still another term, but mass addition of text formats is not something we want to promote; instead going to the listing makes the most sense in the large majority of likely cases (that is, people using the standard installation profile).

That is, submitting admin/config/content/formats/add should take you to admin/config/content/formats (where, among other things, you could change the order in which your new text format will appear with the existing ones).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agentrickard’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
Issue tags: -user experience +#d7ux
FileSize
514 bytes

Simple patch.

Status: Needs review » Needs work
Issue tags: -#d7ux

The last submitted patch, 1053518-format-add.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

#1: 1053518-format-add.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +#d7ux

The last submitted patch, 1053518-format-add.patch, failed testing.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
810 bytes

I see no point to adding a test here.

Status: Needs review » Needs work

The last submitted patch, 1053518-filter-form-redirect.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.64 KB

This bugs me since D7 and I have no idea why I didn't add a redirect there. Verifying this expectation in tests is a piece of cake though :)

However, we might want to wait for #1779026: Convert Text Formats to Configuration System, since I think the changes here conflict with that patch.

Status: Needs review » Needs work

The last submitted patch, drupal8.filter-format-form-redirect.6.patch, failed testing.

sun’s picture

I had a chance to double-check the changes here against #1779026: Convert Text Formats to Configuration System tonight, and the two changes do not conflict with each other, so we can happily move on, once the test failures are fixed.

This patch will only conflict with the follow-up clean-up issue #1881664: Clean up and complete text format config entity conversion. Ideally, we get this patch in before work starts to happen over there.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
1.49 KB

Re-reading the o.p., the tests break because the request was only to redirect when adding a new filter.

This patch adjusts that behavior and fixes the failing tests that result.

I believe the assumption is that updating a filter should return you to the filter form, so you can review changes. Adding a filter should take you to administer filters (or, alternately, to view the filter you just created.)

sun’s picture

mmm, I don't think that makes sense though. The general form submission redirect pattern throughout core is to redirect to two discrete locations, depending on the thing you added/edited:

- for content/viewable entities, we redirect to the view.
- for configuration/unviewable entities, we redirect to the listing page.

We do not make a differentiation between add/edit currently.

The only exception to that rule is the taxonomy term add form, which redirects back to itself, but that's with the concrete idea and purpose that people who are using that form often want to add multiple terms subsequently. It's a relatively bold assumption though, and actually we should remove that special behavior and add a separate "Save and add another" submit button to the term-add form.

For the text format form, there's nothing to review after submitting. In fact, I think the lack of a redirect stems from the D6 user interface of text formats, which had the filter sorting + filter settings as separate tabs/pages and thus, that configuration was not available until a format was saved - hence, no redirect. That's no longer the case though.

agentrickard’s picture

Status: Needs review » Needs work

OK, then we redirect in both instances, and that will mean removing some of the assertFieldByName pieces in the current tests. (Or using the pattern of durpalGet in the later patch).

sun’s picture

Yeah, drupalGet() back to the form. We obviously don't want to remove test coverage. ;)

That said, the assertUrl() changes from #7 should be taken over as well, since that's explicit test coverage for verifying that we end up at the expected URL after posting.

agentrickard’s picture

Got it.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
5.63 KB

With passing tests (locally).

sun’s picture

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -#d7ux +Usability

Glad that it still passes. It's about time to fix this. :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Makes sense. Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)
Issue tags: -Usability

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