Exposed filters eat up too much vertical space on admin/content view

It's indeed the case that there's not enough content of the list to see.
Inlining the button has to be approved by design.
The user behavior of our customers and my teammates is to use the search bar for everything so I don't see people scrolling that list so much.
But you're right - could be better. Any ideas beside of aligning buttons inline?

Quick hint: Also, beside of the filter button, there is a reset button if you have an filter applied in your specific case.
You also have to look for these conditional changes on all pages. I would help to implement whatever fits best.

@mherchel I completely understand your point. Any changes here should first be discussed by the design, so I leave this for now until we have a design decision @saschaeggi on how to improve the situation.

Yes, you're right. Also, in https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/Drupal-Design-system... we have the same design on the page "Fieldset/Details".

As he have now clarified which position the Filter / Reset Buttons should have, we have to think about the other buttons which doesn't come with fieldsets. Spontaneously, I would say that if buttons doesn't come with a fieldset, there are not "inlined".

I would say that this is also related: #3029675 Add support for the inline variation of form elements

Screenshot in #2 looks like a nice improvement and pretty close to the actual design that @mherchel pointed out on #7. It seems like the mockups from the design team are missing actions form elements. I'll try to reach out to them to see if it could be added to the mockups. In the meanwhile, we could work on the changes required to achieve #2 and #7.

Working on it @DevDaysCluj

Hello, attached you can find a patch that implements the behavior in #7

I'm reviewing the patch from #12

claro.theme

function claro_form_views_exposed_form_alter(&$form, FormStateInterface $form_state) {
  $form['#theme_wrappers'][] = 'fieldset';
  $form['#attributes']['class'][] = 'claro-views-exposed-form';
}

1. Can you please add a comment above the function like you have done in the css file describing what this code does?

form.css

form.claro-views-exposed-form .form-actions {
.
.
}

2. I think we don't have to be that specific in that case. As the other definitions in the form.css are also only class and not element based, we can apply that too with only .claro-views-exposed-form .form-actions removing form – Or are there any reasons not to do so?

@rafuel92 Everything else looks good. Thank you very much!!

added patch for this issue. As per shown in #7 image.

@Mahenkvyas22 thank you for your contribution! Not sure if you noticed that this issue has already been worked on before on #12. Given mostly positive feedback, I don't see any reason why we would have to start from scratch.

Thank you for reviewing this @fhaeberle 🙏Besides that, I noticed that we should apply more margin between the last form element and the button.

I also noticed that the there's much more spacing in the bottom of the fieldset than there's in the top.

The red lines are the same size in this picture:

I will work on lauriii's feedback

Adding a new version of the patch, it includes:
- From #15, comment for the function in claro.theme
- From #15, more generic selector for the form-actions, actually it should work for all inline forms, not only views exposed filters
- From #17, double space between items and actions, from 0.5rem to 1rem
- From #18, less bottom space

I also added:
- Moved inline-form styles to a new component inline-form.css
- Removed claro-views-exposed-form class because there is no need of a specific claro class for it.

About the vertical space, I removed it in the fieldset definition as long was added there: https://git.drupalcode.org/project/claro/blob/8.x-1.x/css/src/components... Checked in other fieldsets from the examples in figma and looks nice to me.

About it, @pivica states in the original issue https://www.drupal.org/node/3023291:

One problem we have is with form fields - they are defining the top and bottom margin which does not plays well with parent wrappers which are defining inner padding. One solution for this is to apply an only bottom margin to form items and reset bottom margin for last form element - Bootstrap is doing similar, they are only defining a bottom margin to avoid similar problems.

So I'm not sure if is better to keep the bottom margin in fieldset and force remove it in the items and action here, or remove it in the fieldset and let the items add it their own. IMHO this should be reviewed when the responsive is implemented.

@quiron Thanks for your work, looks good so far but RTL isn't covered. Can you adjust that?

I'm not really used to work with RTL, I did my best but someone with more experience should look into it.

BTW, I'm wondering if the styles inside inline-forms.css should be scoped to work only inside fieldsed tags or should be applied to all inline forms.

@quiron Thanks for your help! I applied minor changes to your work. I removed some of your comments because they were obvious and I think the scope of the selector is just fine and can be leaved like this.
Can you review my changes?

To me, the comment clarifies why clear: none; is repeated, but yes, quite obvious.

LGTM.

1. 

   This breaks fieldsets on other pages. For example, on /admin/structure/views/add View settings has wrong padding in the bottom of the fieldset.

2. +++ b/css/src/components/inline-form.css
   @@ -0,0 +1,19 @@
   +fieldset .form--inline .form-actions {
   ...
   +[dir="rtl"] fieldset .form--inline .form-actions {
   

   We should target .fieldset instead of the element type.

3. +++ b/css/src/components/inline-form.css
   @@ -0,0 +1,19 @@
   +  float: left;
   

   We should add LTR comment here as well.

4. +++ b/templates/fieldset.html.twig
   @@ -46,9 +46,11 @@
   +  {% if legend.title %}
   

   👍

Fixed 2 and 3.

About 1, this case is container-inline and not form--inline. It also looks like your screenshot with and without any patch of this issue. IMHO this should be addressed here: #2417111: Replace container-inline with form--inline to display forms horizontally.

Based on #29 it seems like #28.1 is already broken prior to this change, so moving to needs review.

Tested manually and I don't have any remaining concerns!

Committed and pushed! Thank you again!

Thank you @lauriii for coordinating, onboarding and supporting!

I decided to revert the commit (from #29) and repopen this issue, because:

1. The patch adds a regression for the fieldset component styles. Screenshots here.
2. The actually added fieldset duplicates the id of the exposed form.
3. If a user adds an exposed form block to a region, core begins complaining about PHP warnings and notices.
4. The patch does not care about narrow screens.

My suggestions/observations:

1. The issue can be solved without an additional element by providing a standalone component file for .views-exposed-form. We can do that here.
2. Although there is some guidance here, it isn't specified well and doesn't say anything about narrow screens. For the final outcome we need a spec for views exposed filters.
3. It is unclear (at least for me) what's the intention here. I guess that the change should affect more exposed forms, and not only the Content views form. But what should happen if a user needs the views exposed form in a block? Should the change affect those cases as well?

@huzooka thank you for testing this and posting feedback here! Regarding #38.4, we discussed this at dev days and decided to ignore the narrow screen customizations for now. It seems like we forgot to post that here and to open a follow-up for that. Let's open a separate issue for discussing that.

Can somebody help: What's left here?
This?

The issue can be solved without an additional element by providing a standalone component file for .views-exposed-form.

Comment #38 is a summary of what needs to be done. See #3063096: Regression: Fieldset styles not follow the design anymore & PHP warnings and notices for more details.

Given the issues mentioned above, I feel that these patches may be approaching the problem a bit too complexly. If all you need is for the styling to fall in line (pun intended) with the design & save space, really all you need is to apply some flexbox styling to the inline form.
There isn't really a reason to force a fieldset in there. Sure, you can argue 'fieldset' should be in there because it groups fields, but I don't see the added value vs. the problems created.

I've included a patch that does just the inline styling + makes the 'Actions' go inline as well. (tested with alpha 3)
I'm sure it can use some fine-tuning, but it's what I need to get the theme to a level that works for my team, so I'm adding it here for anyone else needing a practical fix.

EDIT: Not sure why the tests keep failing while I can apply the latest ones (46) without issue locally. Will try to debug further.

I added the wrong patch, sorry about that.
Here is the right one (43)

Alright, attempt number 3 to get the right one!

And once more

For what it's worth, here's a patch on release alpha3 + a patch for 8.x-1.x
Hope it helps someone out.

Reviewing this...

Re #47:

After discussing with @lauriii, we may decide to still use Fieldset here for accessibility reasons (we're investigating this option). If we won't do that, I think that this is a right approach. Thanks for the patch (and for working on this)!

Review:

1. Variables needs to be updated:
   • --size-details-border → --details-border-size
   • --color-details-border → --details-border-color
   • --base-border-radius → --details-border-size-radius
   • --shadow-details → --details-box-shadow

2. +++ b/css/src/components/inline-form.css
   @@ -0,0 +1,42 @@
   + * Visual styles for inline forms.
   

   Visual styles for views exposed form?

3. +++ b/css/src/components/inline-form.css
   @@ -0,0 +1,42 @@
   +.views-exposed-form .form--inline {
   ...
   +  float: none;
   ...
   +  clear: none;
   

   What if we just remove the .form-inline class form the views-exposed-filter.html.twig template and from all of the selectors here? We reuse almost nothing from the original .form--inline styles.

   I'd add margin-top: var(--space-l); and margin-bottom: var(--space-l); here as well.

4. +++ b/css/src/components/inline-form.css
   @@ -0,0 +1,42 @@
   +[dir="rtl"] .views-exposed-form .form--inline {
   +  order: revert;
   +}
   

   Invalid (and unneeded) value and selector.

5. +++ b/css/src/components/inline-form.css
   @@ -0,0 +1,42 @@
   +  margin: var(--space-s) 0 0 var(--space-xs);
   

   /* LTR */ comment missing.

6. +++ b/css/src/components/views-ui.css
   @@ -16,6 +16,33 @@ details.fieldset-no-legend {
   +/* make the form elements in the header of an overview go inline, eg. 'Action' on admin/content */
   +
   +.view-content .form-wrapper {
   +  display: flex;
   +  flex-wrap: wrap;
   +  align-content: flex-end;
   +  margin-top: var(--space-xs);
   +}
   +
   +.view-content .form-wrapper .form-item {
   +  margin-top: var(--space-s);
   +  margin-bottom: 0;
   +}
   +
   +.view-content .form-wrapper .form-actions {
   +  margin: var(--space-s) 0 0 var(--space-xs);
   +}
   +
   +[dir="rtl"] .view-content .form-wrapper .form-actions {
   +  margin: var(--space-s) var(--space-xs) 0 0;
   +}
   +
   +.view-content .form-wrapper .form-actions .button {
   +  margin-top: var(--space-s);
   +  margin-bottom: 0;
   +}
   +
   

   If these are trying to format the Entity Operations Bulk form, then these should go into a separate file.

   Views UI can be disabled, and in that case these styles won't be applied.

   Anyway, I'd try to handle these by custom classes instead of this (maybe too general) SMACSS.

Addressed the feedback in #49, including the addition of custom classes so css selectors don't need to be as lengthy.

Went ahead and built on the approach from @lordrembo as fieldset in the views exposed filter form wouldn't provide any additional accessibility benefits. The filter inputs are not a subset of a larger form, so grouping them with fieldset would not accomplish anything that isn't already accomplished by them being inside the same <form> tag.

This did highlight some accessibility issues on the page, and this patch makes a first attempt at addressing them. What I did:

• Added an aria-label to the exposed filter, so the purpose of the form is clearer to screenreaders that haven't yet scanned the content of the view.
• Added the 'group' role to the bulk actions container (and added a screenreader-only label), as this should be interpreted like a fieldset that contains the select + submit elements.

Also worth mentioning that I removed the top and bottom margin from .action-links .button, and added a new claro-specific action links css file to do this. Removing these margins gets the action links much closer to the Figma designs. Based on the other pages I visited, this change in margin has no visual difference outside of its use in admin/content, as those margins are collapsed in all the other instances I found.

In review.

Re #52:

Thanks for the accessibility direction and for the patch! Excellent work! 👍👌🐼

I've found only one thing:

+++ b/claro.info.yml
@@ -57,6 +57,7 @@ libraries-override:
+        css/components/action-links.css: false

+++ b/claro.libraries.yml
@@ -7,6 +7,7 @@ global-styling:
+      css/dist/components/action-links.css: {}

+++ b/css/src/components/action-links.css
@@ -0,0 +1,47 @@
+.action-links .button {
+  margin-top: 0;
+  margin-bottom: 0;
+}

As I see, the only reason for replacing the inherited action-links component styles was to add this .action-links .button selector to override button styles – but this should be only .button-action.

If we still want to have this added (this is out of the scope of this issue - right now Claro lacks action links design #3036742: CTA - Call to action component), then I'd convert this .button-action selector to a real modifier class .button--action (see this) and move it into our button component file.

(Now the styles provided by the .button-action selector are overridden by the styles that are coming from our button.css, but only because it follows this new action-link.css component in the libraries.yml of this patch.)

We must either solve this problem (for example, on the basis of the suggestions above) or we should remove action-link related things from the patch.

I forgot assigning this to myself.

This patch addresses #54, and:

• Uses BEM classes for views exposed form
• Uses BEM classes for views bulk operation

Cool to see more progress on this! Gonna try the new stuff out when I find some time.

1. +++ b/claro.theme
   @@ -486,6 +493,111 @@ function claro_form_view_edit_form_alter(&$form, FormStateInterface $form_state)
   +            $form['header'][$key][$bulk_child_before_actions_key]['#wrapper_attributes']['class'][] = 'views-bulk-actions__item--actions-after';
   ...
   +    $form[$child_before_actions_key]['#wrapper_attributes']['class'][] = 'views-exposed-form__item--actions-after';
   

   Actions after is a bit confusing name for this variation. Maybe something like --last-input or --field-preceding-actions. We might also want to consider just using :nth-last-child(2) given that this is about visual representation and we might not be able to come up with a class name that would make sense.

2. +++ b/css/src/components/field-ui.css
   @@ -1,7 +1,4 @@
   -/**
   - * @file
   - * Field UI admin styles.
   - */
   +/* Field UI */
   
   @@ -15,6 +12,9 @@
   +#field-display-overview .field-plugin-settings-edit-form .form-item {
   +  margin: 10px 0;
   +}
   

   This seems like an unrelated change.

Re #58:

1. There can be other elements in the form (for example, hidden inputs) that aren't visible, and I'd like to keep this solution as flexible as possible. I kept the CSS class based solution, the new element modifier classes are .views-exposed-form__item--preceding-actions and .views-bulk-actions__item--preceding-actions
2. I think this is the result of a faulty rebase. Fixed.

Discussed with @huzooka on Slack that we should remove bulk operation related changes from the patch because this doesn't play nicely with VBO module:

Let's open a follow-up to implement the bulk operation designs in a way that is compatible with this contrib module.

Addressing #61.

Follow-up: #3070558: Implement bulk operation designs.

This is a great improvement. Thank you everyone! And thank you @huzooka for pushing this to the finish line and @bnjmnm for the accessibility review and improvements 🙏 Committed and pushed!