(please do not commit - to be used (maybe) for a live commit at dev days)

Suggest commit message:
git commit -m 'Issue #2470137 by tadityar, zaporylie, pec, LewisNyman, ry5n, Bojhan, yoroy: Style Seven'\''s fieldset elements'

Problem/Motivation

Seven's fieldset elements are unstyled, see the screenshots in #1493324: Inline form errors for accessibility and UX

Proposed resolution

Implement the styling from the styleguide https://groups.drupal.org/node/283223#Fieldset

In #2002336: Introduce a CSS class to hide borders of fieldset elements we introduced a class called fieldgroup that unstyles fieldsets which means we can use fieldsets for accessibility without affecting the visuals.

Previously all fieldsets had margin styling even if they had the fieldgroup class applied.

This patch removes margin styling from fieldsets with the fieldgroup class in Seven. This does not cause any visual regressions because that the fieldsets with the class fieldgroup also had the class form-item which applies margin styling that overwrote the fieldgroup margins so they never applied.

Remaining tasks

Style fieldset elements
Download and install vijaycs85's Errorstyle module from https://github.com/vijaycs85/errorstyle and go to /error-style/form to test a fieldset
add before and after screenshots to the issue summary

User interface changes

Fieldsets look nicer!

API changes

None

Before:

After:

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because styleguide implementation
Issue priority Normal priority because it' an isolated incident, we don't use fieldset elements that often in core.
Unfrozen changes Unfrozen because it only changes CSS
CommentFileSizeAuthor
#23 Screenshot 2015-04-16 18.40.45.jpg275.35 KBLewisNyman
#21 Screenshot 2015-04-16 11.08.12.jpg778.98 KBLewisNyman
#21 Screenshot 2015-04-16 11.07.13.jpg816.69 KBLewisNyman
#16 style_seven_s_fieldset-2470137-16.patch653 byteszaporylie
#13 Screenshot 2015-04-14 17.46.18.jpg264.79 KBLewisNyman
#13 Screenshot 2015-04-14 17.46.18.jpg264.79 KBLewisNyman
#10 style-seven-fieldset-2470137.patch652 bytespec
#7 interdiff-5-7.txt586 bytestadityar
#7 style-seven-fieldset-2470137-7.patch646 bytestadityar
#5 style-seven-fieldset-2470137-5.patch649 bytestadityar
#4 Screen Shot 2015-04-14 at 4.50.49 PM.png34.1 KBtadityar
#4 style-seven-fieldset-2470137-4.patch947 bytestadityar
#2 Screen Shot 2015-04-14 at 4.18.05 PM.png33.82 KBtadityar
#2 style-seven-fieldset-2470137-2.patch947 bytestadityar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Assigned: Unassigned » tadityar

Working on this

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Active » Needs review
FileSize
style-seven-fieldset-2470137-2.patch947 bytes
Screen Shot 2015-04-14 at 4.18.05 PM.png33.82 KB

Initial attempt.

Somehow I can't get the "Source Sans Pro" font to work and thus still confused on how to get this one semibolded and from the screenshot 15px/24px is too small for the legend. Any suggestion on other size?

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work

There are some differences between the original proposal and in implementation. It wasn't so simple to add the webfont to core. See: #1986082: Add a webfont version of Source Sans Pro font
Because we don't have the same font, we can't use the same font sizes as the proposal. I would try using 1em instead.

Also:

  1. +++ b/core/themes/seven/css/components/form.css
@@ -6,7 +6,21 @@ form {
+  padding-top: 30px;
+  position: relative;
+
+}

    There is an empty line here that we need to remove 

  2. +++ b/core/themes/seven/css/components/form.css
@@ -85,12 +99,15 @@ label[for] {
 /* Filter */
+.filter-wrapper {
+  font-size: 0.923em;
+}
...
-  font-size: 0.95em;
+  font-size: 0.9em;
 }

    Are these changes related to this issue?

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Needs work » Needs review
FileSize
style-seven-fieldset-2470137-4.patch947 bytes
Screen Shot 2015-04-14 at 4.50.49 PM.png34.1 KB

Fixed #1, yes #2 is not related to this issue.. accidentally got there :)

New screenshot attached.

tadityar’s picture

tadityar CreditAttribution: tadityar commented
FileSize
style-seven-fieldset-2470137-5.patch649 bytes

err.. I edited the wrong file sorry

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work

Looking good! Thanks.

  1. +++ b/core/themes/seven/css/components/form.css
@@ -6,7 +6,20 @@ form {
 fieldset {

    We should replace this selector with fieldset:not(.fieldgroup) because we introduced this unstyled class in this issue #2408481: Rewrite help.module component's inline with our CSS standards 

  2. +++ b/core/themes/seven/css/components/form.css
@@ -6,7 +6,20 @@ form {
+  font-family: 'Source Sans Pro','Lucida Grande', sans-serif;

    We should remove this font-family declaration

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Needs work » Needs review
FileSize
style-seven-fieldset-2470137-7.patch646 bytes
interdiff-5-7.txt586 bytes
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work

Excellent! I had a look at this patch applied and I think the padding on the left and right need to be tweaked.

+++ b/core/themes/seven/css/components/form.css
@@ -5,8 +5,20 @@ form {
+  padding-top: 30px;

Instead can we use something like: padding: 30px 18px 18px;

After that we are 100% good.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Assigned: tadityar » Unassigned
Issue tags: +Novice

Tagging novice because the remaining task is simple enough for someone to work on at Drupal Dev Days

pec’s picture

pec CreditAttribution: pec commented
FileSize
style-seven-fieldset-2470137.patch652 bytes

Hi, this is my first try at contributing to drupal for this drupaldevdays mentoring sprint.
I think this should be ok.

keyral’s picture

keyral CreditAttribution: keyral commented
Status: Needs work » Needs review
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
Screenshot 2015-04-14 17.46.18.jpg264.79 KB
Screenshot 2015-04-14 17.46.18.jpg264.79 KB

This looks good to me! Thanks

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/seven/css/components/form.css
@@ -5,8 +5,20 @@ form {
+fieldset:not(.fieldgroup) legend{

I think we need a space after legend

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
Assigned: Unassigned » zaporylie

Will fix it.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
Status: Needs work » Reviewed & tested by the community
FileSize
style_seven_s_fieldset-2470137-16.patch653 bytes

Bumping it back to RTBC.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Reviewed & tested by the community » Needs review

RTBC++

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community

Urgh sorry, back to RTBC

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
Assigned: zaporylie » Unassigned

yeah... I left it RTBC since change was so small.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/seven/css/components/form.css
@@ -5,8 +5,20 @@ form {
-fieldset {
+fieldset:not(.fieldgroup) {
...
   margin: 1em 0;

Doesn't this change the .fieldgroup margins. Was that intended?

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
FileSize
Screenshot 2015-04-16 11.07.13.jpg816.69 KB
Screenshot 2015-04-16 11.08.12.jpg778.98 KB

Good catch. Funnily enough it doesn't. The margins never applied to fieldgroups because it was being overridden by form-item. I'm happy to have fieldgroup as a completely unstyled class, and let form-item handle spacing for consistency.

Before:

After:

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes
Issue tags: +Needs issue summary update, +drupaldevdays

discussed at dev days

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
FileSize
Screenshot 2015-04-16 18.40.45.jpg275.35 KB
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
Issue tags: -Needs issue summary update

I've added a suggested commit message to the top of the issue summary that includes credit to the original designers of the Seven style guide:
git commit -m 'Issue #2470137 by tadityar, zaporylie, pec, LewisNyman, ry5n, Bojhan, yoroy: Style Seven'\''s fieldset elements'

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Assigned: Unassigned » alexpott
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed a1717dc and pushed to 8.0.x. Thanks!

  • alexpott committed a1717dc on 8.0.x 
    Issue #2470137 by tadityar, zaporylie, pec, LewisNyman, ry5n, Bojhan,...

Status: Fixed » Closed (fixed)

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