Problem/Motivation

For binary options we should use a checkbox, not a "yes" "no" radio.

Proposed resolution

The checkbox can say "Exposed form in separate block"

The description could likely also be added to the bottom of the checkbox (not on top). Saying "Removes exposed form from view. Creates disabled block on blocks page ". Where "blocks page" is linked.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aburrows’s picture

Assigned: Unassigned » aburrows

I'm a mentor and working on this issue at DC LA

pguillard’s picture

Oops, I started to work on it too. I let you make the patch, and maybe I'll review it!

pguillard’s picture

All right, just because I made a patch, works ok, and passes test. Hum..will be for next time..

aburrows’s picture

Submit your patch, and I'll test if it works will save me from doing it

aburrows’s picture

aburrows’s picture

aburrows’s picture

Status: Active » Needs review
Bojhan’s picture

lauriii’s picture

Status: Needs review » Needs work
Issue tags: +Needs screenshots
aburrows’s picture

Screenshot attached again

aburrows’s picture

Status: Needs work » Needs review
pguillard’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
26.99 KB
27.01 KB

I reviewed your patch, it is working fine.
Added 2 more screenshots when checkbox checked/not checked.

lauriii’s picture

RTBC++

lauriii’s picture

Assigned: aburrows » Unassigned
xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs screenshots

Thanks for the manual testing and the screenshots! I've embedded the main screenshot in the UI changes section of the summary for easier review.

So the title of the modal is "Put the exposed form in a block" and the checkbox is "Expose form in separate block". The "Put" is a little weird as a header anyway, and the checkbox text can be misread since it's missing some articles (I blame English for this).

Is there any reason not to make them say the same thing, i.e. (with fewer missing articles), "Expose the form in a separate block" for both? @Bojhan, others, thoughts? Setting NR for feedback on that suggestion.

As an aside, we should add beta evaluations for normal tasks, in general. In this case, since this currently only changes UI strings, it's unfrozen. Thanks!

aburrows’s picture

@xjm I agree, when discussing this issue with several others at DC they seemed confused by the term Put, we can change to something like "Display exposed form as a block"

aburrows’s picture

Updated patch so modal title says: "Display exposed form as a block" and the checkbox #title says "Expose form in separate block".
As per discussion with xjm.

Bojhan’s picture

Hmm, well we refer throughout the interface to a "exposed form" which is why I put it like that. We can diverge that is fine.

aburrows’s picture

Issue tags: +Needs beta evaluation
Zerdiox’s picture

I've checked this at Drupalcon Barcelona 2015 against the latest beta version.
Patch still works and can be implemented.

I'll second adding the description under the checkbox that is proposed: Removes exposed form from view. Creates disabled block on blocks page

This behaviour is sometimes not expected by sitebuilders and is a good reminder. I've also added this as a new patch, should this not be necessary the patch from comment 17 works as is.

sam0971’s picture

Status: Needs review » Reviewed & tested by the community

I tested the patch from #20 and it works. Checkbox is appearing in exposed filter settings.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: exposed-views-form-checkbox-2489406-20.patch, failed testing.

Status: Needs work » Needs review
aburrows’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
102.76 KB

Both patches work, RTBC++

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +rc deadline

Thanks for your work on this issue!

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -1771,14 +1771,15 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+          '#description' => $this->t('Removes exposed form from view. Creates disabled block on blocks page.')

Why are we adding this description? I don't think we should. It seems out of scope for the issue and we have done a lot of work to remove unnecessary descriptions in the Views UI.

Since this issue is a string change, it has an rc deadline, so tagging so that it will get more visibility in case the patch is updated before then.

xjm’s picture

Issue summary: View changes
FileSize
120.31 KB

Ah, I think I understand why the description was added (re-reading the summary). We should only have one description on the form, not both. Illustration attached.

I also think the suggested description is a bit too confusing and terse, so maybe it would be better to change the form element only (so merely remove that hunk from the patch) and then change the description in a separate issue where we discuss the improved wording. Note that this change may also be added in 8.1.x if it's not done by RC.

pguillard’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.45 KB

@xjm : Applied the quick remove as of #25 because it is needed for RC deadline.
I allow myself to set it to RTBC, as it was before.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: exposed-views-form-checkbox-2489406-27.patch, failed testing.

catch’s picture

Title: Put the exposed form should be a checkbox. » Put the exposed form in a block should be a checkbox.
catch’s picture

Issue tags: +minor version target

This is the sort of change we can make in a minor version, so if it misses the release candidate deadline it'd be fine for 8.1.x or later. Tagging 'minor version target'.

Also it doesn't seem 100% necessary to change the existing string in order to make this change.

pguillard’s picture

dawehner’s picture

So what I don't understand is why we cannot change strings when we also provide an update path for them at the same time? Wouldn't that solve the issues with existing translations?

The last submitted patch, 27: exposed-views-form-checkbox-2489406-27.patch, failed testing.

pguillard’s picture

@Catch : I didn't see your comments..

pguillard’s picture

Himanshu5050’s picture

Reviewed patch from #34 and it is working fine.minor change applied i.e.
Exposed form should be displayed as a block (Screen 02) not in a block (Screen 01).

Screen 01: Screen 01

Screen 02: Screen 02

Himanshu5050’s picture

subhojit777’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
8.96 KB
20.75 KB
9.17 KB
21.34 KB

Before applying patch:

After applying patch:

Patch looks good, hence RTBC :)

Bojhan’s picture

Version: 8.0.x-dev » 8.1.x-dev
mikeker’s picture

dawehner’s picture

It is indeed. I don't care which one is committed, at all.

cilefen’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

Since the maintainer doesn't care, I prefer the checkbox. However, being newer, this is a duplicate of #1969594: Make "expose form in block" UI easier to understand. Can those who have been working on this post a patch over there?