When viewing the default admin/content theme within Claro, I noticed that I'm not able to see any relevant content without scrolling. For reference I'm on a 23 inch / 58cm monitor with a 1920x1080 resolution. When viewing with Seven, I can see 9 rows of content.
This might be confusing for an editor, who might not expect to have to scroll initially. In addition, the requirement of scrolling on this commonly used view adds to additional "work".
I'm not sure if the design system can accommodate something a little more compact, but if it can, I believe it's needed.
Proposed design by the design team:
Comment | File | Size | Author |
---|---|---|---|
#63 | interdiff-3055598-60-63.txt | 5.08 KB | huzooka |
#63 | claro-views_exposed_filters-3055598-63.patch | 7.43 KB | huzooka |
| |||
#61 | Screen Shot 2019-07-26 at 15.58.10.png | 154.48 KB | lauriii |
#60 | interdiff-3055598-56-60.txt | 2.25 KB | huzooka |
#60 | claro-views_exposed_filters-3055598-60.patch | 11.94 KB | huzooka |
|
Comments
Comment #2
mherchelPlaying around in Developer Tools, I moved the exposed filters' buttons to the right of the forms, and also removed some double margins (where each of multiple layers of elements had margins). It looks better in my opinion.
Of course doing this on one view, in dev tools is much easier than making a global styling change. But, if this looks okay to everyone, I can work on a patch. Thoughts?
Comment #3
fhaeberleIt'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.
Comment #4
mherchelNo, the content is there. I can scroll to reach it. It's being displaced by the exposed filters.
The only other ideas would affect the design system: decrease the heights of the buttons and vertical margins.
Comment #5
mherchelAnother option would be to throw the exposed filters into a sidebar. But that is obviously more involved than a styling change.
Comment #6
fhaeberle@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.
Comment #7
mherchelHmmm... looking through the initial design issue #3017785: Designs for a new admin theme, it appears that the buttons are supposed to be inline. Here's the comp off of that issue:
Comment #8
fhaeberleYes, 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
Comment #9
fhaeberleComment #10
lauriiiScreenshot 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.
Comment #11
rafuel92 CreditAttribution: rafuel92 as a volunteer and at Ibuildings commentedWorking on it @DevDaysCluj
Comment #12
rafuel92 CreditAttribution: rafuel92 as a volunteer and at Ibuildings commentedHello, attached you can find a patch that implements the behavior in #7
Comment #13
rafuel92 CreditAttribution: rafuel92 as a volunteer and at Ibuildings commentedComment #14
fhaeberleI'm reviewing the patch from #12
Comment #15
fhaeberleclaro.theme
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
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
removingform
– Or are there any reasons not to do so?@rafuel92 Everything else looks good. Thank you very much!!
Comment #16
Mahenkvyas22 CreditAttribution: Mahenkvyas22 as a volunteer commentedadded patch for this issue. As per shown in #7 image.
Comment #17
lauriii@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.
Comment #18
lauriiiI 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:
Comment #19
quironI will work on lauriii's feedback
Comment #20
quironAdding 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:
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.
Comment #21
quironComment #22
fhaeberle@quiron Thanks for your work, looks good so far but RTL isn't covered. Can you adjust that?
Comment #23
quironI'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 insidefieldsed
tags or should be applied to all inline forms.Comment #24
quironComment #25
fhaeberleComment #26
fhaeberle@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?
Comment #27
quironTo me, the comment clarifies why
clear: none;
is repeated, but yes, quite obvious.LGTM.
Comment #28
lauriiiThis breaks fieldsets on other pages. For example, on
/admin/structure/views/add
View settings has wrong padding in the bottom of the fieldset.We should target
.fieldset
instead of the element type.We should add LTR comment here as well.
👍
Comment #29
quironFixed 2 and 3.
About 1, this case is
container-inline
and notform--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.Comment #30
lauriiiBased on #29 it seems like #28.1 is already broken prior to this change, so moving to needs review.
Comment #31
lauriiiTested manually and I don't have any remaining concerns!
Comment #33
lauriiiCommitted and pushed! Thank you again!
Comment #34
quironThank you @lauriii for coordinating, onboarding and supporting!
Comment #35
rosinegrean CreditAttribution: rosinegrean at PitechPlus commentedComment #37
huzookaComment #38
huzookaI decided to revert the commit (from #29) and repopen this issue, because:
My suggestions/observations:
.views-exposed-form
. We can do that here.Comment #39
lauriii@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.
Comment #40
fhaeberleCan somebody help: What's left here?
This?
Comment #41
lauriiiComment #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.
Comment #42
rembrandx CreditAttribution: rembrandx at Dropsolid commentedGiven 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.
Comment #43
rembrandx CreditAttribution: rembrandx at Dropsolid commentedI added the wrong patch, sorry about that.Here is the right one (43)
Comment #44
rembrandx CreditAttribution: rembrandx at Dropsolid commentedAlright, attempt number 3 to get the right one!Comment #45
rembrandx CreditAttribution: rembrandx at Dropsolid commentedAnd once moreComment #46
rembrandx CreditAttribution: rembrandx at Dropsolid commentedFor what it's worth, here's a patch on release alpha3 + a patch for 8.x-1.x
Hope it helps someone out.
Comment #47
rembrandx CreditAttribution: rembrandx at Dropsolid commentedComment #48
huzookaReviewing this...
Comment #49
huzookaRe #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:
--size-details-border
→--details-border-size
--color-details-border
→--details-border-color
--base-border-radius
→--details-border-size-radius
--shadow-details
→--details-box-shadow
Visual styles for views exposed form?
What if we just remove the
.form-inline
class form theviews-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);
andmargin-bottom: var(--space-l);
here as well.Invalid (and unneeded) value and selector.
/* LTR */ comment missing.
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.
Comment #50
huzookaComment #51
bnjmnmComment #52
bnjmnmAddressed 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:
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.Comment #53
huzookaIn review.
Comment #54
huzookaRe #52:
Thanks for the accessibility direction and for the patch! Excellent work! 👍👌🐼
I've found only one thing:
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 ourbutton.css
, but only because it follows this newaction-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.
Comment #55
huzookaI forgot assigning this to myself.
Comment #56
huzookaThis patch addresses #54, and:
Comment #57
rembrandx CreditAttribution: rembrandx at Dropsolid commentedCool to see more progress on this! Gonna try the new stuff out when I find some time.
Comment #58
lauriiiActions 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.This seems like an unrelated change.
Comment #59
huzookaComment #60
huzookaRe #58:
.views-exposed-form__item--preceding-actions
and.views-bulk-actions__item--preceding-actions
Comment #61
lauriiiDiscussed 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.
Comment #62
huzookaComment #63
huzookaAddressing #61.
Follow-up: #3070558: Implement bulk operation designs.
Comment #65
lauriiiThis 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!