More uniformity in form elements markup #6356

Closed
hypeJunction opened this Issue Jan 26, 2014 · 23 comments

Comments

Projects
None yet
4 participants
@hypeJunction
Contributor

hypeJunction commented Jan 26, 2014

Perhaps, we should start moving towards a more uniform markup of form fields, so that they are consistent across the core and third-party plugins. Currently there is not much convention, so we end up with forms that are very hard to style.

I believe we could use a wrapper view, which would use the following:

  • label text
  • input view
  • help text
  • validation result placeholder/text

Research:

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jan 26, 2014

Member

Something similar to what bootstrap and other css frameworks have for forms would be fantastic, yea

Member

ewinslow commented Jan 26, 2014

Something similar to what bootstrap and other css frameworks have for forms would be fantastic, yea

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jan 26, 2014

Contributor

I will try to come up with a prototype

Contributor

hypeJunction commented Jan 26, 2014

I will try to come up with a prototype

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jan 27, 2014

Contributor

Here are some of the items that contribute to inconsistencies:

  1. Use of colons in label strings - I think there should be a general agreement on whether or not to use colons (e.g. Compose a message form uses them, while most others don't)
  2. Positioning of <select> tags with relation to the label is inconsistent, it jumps around from being inline to being under the label. I think this should be controlled by the form class, e.g. .elgg-form-inline
  3. <br /> tags should be eliminated across all forms, this should controlled via css
  4. <label> tags should be used at all times - they are omitted in user settings form and notifications forms
  5. Consider using <fieldset> in stead of .elgg-module in usersettings forms
  6. We could make forms more extendable by replacing .elgg-foot with <fieldset class="hidden"> for hidden inputs and <fieldset class="elgg-foot"> for buttons. The latter could become it's own view (Cancel button with JS to go back or to close an open modal, Reset to reset the form, Save - all with options to disable and to change labels)

Here is a first draft shot at some of these. https://gist.github.com/hypeJunction/8647368#file-forms-php
You can just overwrite the theme_sandbox/forms view to see it in action. I have added custom styles for the grid selectors to make the form responsive.

Contributor

hypeJunction commented Jan 27, 2014

Here are some of the items that contribute to inconsistencies:

  1. Use of colons in label strings - I think there should be a general agreement on whether or not to use colons (e.g. Compose a message form uses them, while most others don't)
  2. Positioning of <select> tags with relation to the label is inconsistent, it jumps around from being inline to being under the label. I think this should be controlled by the form class, e.g. .elgg-form-inline
  3. <br /> tags should be eliminated across all forms, this should controlled via css
  4. <label> tags should be used at all times - they are omitted in user settings form and notifications forms
  5. Consider using <fieldset> in stead of .elgg-module in usersettings forms
  6. We could make forms more extendable by replacing .elgg-foot with <fieldset class="hidden"> for hidden inputs and <fieldset class="elgg-foot"> for buttons. The latter could become it's own view (Cancel button with JS to go back or to close an open modal, Reset to reset the form, Save - all with options to disable and to change labels)

Here is a first draft shot at some of these. https://gist.github.com/hypeJunction/8647368#file-forms-php
You can just overwrite the theme_sandbox/forms view to see it in action. I have added custom styles for the grid selectors to make the form responsive.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jan 27, 2014

Member
  1. I vote no colons
  2. Agreed
  3. Agreed
  4. Definitely. We also need to make sure they're semantically associated with the control.
  5. We can use both?
  6. Hmmm... Tougher issue. May change dramatically with move toward Angular.
Member

ewinslow commented Jan 27, 2014

  1. I vote no colons
  2. Agreed
  3. Agreed
  4. Definitely. We also need to make sure they're semantically associated with the control.
  5. We can use both?
  6. Hmmm... Tougher issue. May change dramatically with move toward Angular.
@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jan 28, 2014

Contributor

p2. Should we add the .elgg-form-inline class or do we leave it to the plugins to implement if they need it?
p5. Yes, I don't see why we can't use both. This would provide a markup that easier to style. Question which tag wraps the other? <fieldset><div class="elgg-module"></div></fieldset> or the other way around?
p6. The only reason I mention this is that it seems that .elgg-foot implies presence of .elgg-head and .elgg-body, which is not the case with forms. In one of the recent projects, we were delivering a number of forms via modals, which had a header, a body, and a footer, each of which required distinct styling. I had to wrap form body into .elgg-body to make this work, otherwise the body of the form was not stylable. This could have been achieved with negative margins, but I don't trust those much.

Other items:
p7. Do we want to add form state classes, similar to what I have in the gist?

Contributor

hypeJunction commented Jan 28, 2014

p2. Should we add the .elgg-form-inline class or do we leave it to the plugins to implement if they need it?
p5. Yes, I don't see why we can't use both. This would provide a markup that easier to style. Question which tag wraps the other? <fieldset><div class="elgg-module"></div></fieldset> or the other way around?
p6. The only reason I mention this is that it seems that .elgg-foot implies presence of .elgg-head and .elgg-body, which is not the case with forms. In one of the recent projects, we were delivering a number of forms via modals, which had a header, a body, and a footer, each of which required distinct styling. I had to wrap form body into .elgg-body to make this work, otherwise the body of the form was not stylable. This could have been achieved with negative margins, but I don't trust those much.

Other items:
p7. Do we want to add form state classes, similar to what I have in the gist?

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jan 28, 2014

Contributor

p2. I don't think we need to add an actual class. Once <br /> tags are removed, we can do the following:

label + select {
   margin-left: 10px;
}
select[multiple] {
   display: block;
   margin: 0;
}

and remove styling from .elgg-input-access

p5. It made more sense to wrap module content in <fieldset>, so that fieldset > div rule kicks in

Contributor

hypeJunction commented Jan 28, 2014

p2. I don't think we need to add an actual class. Once <br /> tags are removed, we can do the following:

label + select {
   margin-left: 10px;
}
select[multiple] {
   display: block;
   margin: 0;
}

and remove styling from .elgg-input-access

p5. It made more sense to wrap module content in <fieldset>, so that fieldset > div rule kicks in

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Feb 8, 2014

Member

For fieldset I was thinking:

<fieldset class="elgg-module">
  <legend class="elgg-head"></legend>
  <div class="elgg-body"></div>
</fieldset>
Member

ewinslow commented Feb 8, 2014

For fieldset I was thinking:

<fieldset class="elgg-module">
  <legend class="elgg-head"></legend>
  <div class="elgg-body"></div>
</fieldset>
@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Feb 8, 2014

Member

One thing Elgg has very little of that I want to see more of going forward is inline error messages. Our structure should take those into account from the get go.

Member

ewinslow commented Feb 8, 2014

One thing Elgg has very little of that I want to see more of going forward is inline error messages. Our structure should take those into account from the get go.

@ewinslow ewinslow closed this Feb 8, 2014

@ewinslow ewinslow reopened this Feb 8, 2014

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Feb 8, 2014

Member

Also, in keeping with a mobile first philosophy, let's just worry about vertically stacked forms for now:

  • Inline forms or right aligned forms generally aren't good on mobile.
  • Stacked forms are the most usable on any platform.
  • They're the easiest to get right from an i18n perspective.

Proposed markup for single field inputs:

<label class="elgg-input-wrapper">
  <span class="elgg-input-label">Label</span>
  <input>
  <ul>
    <li>Error messages</li>
  </ul>
</label>

And for checkboxes and radios:

<fieldset class="elgg-input-wrapper">
  <legend class="elgg-input-label">Label</legend>
  <ul class="elgg-input-checkboxes">
    <li><label><input type="checkbox">Checkbox label</label></li>
  </ul>
</fieldset>
Member

ewinslow commented Feb 8, 2014

Also, in keeping with a mobile first philosophy, let's just worry about vertically stacked forms for now:

  • Inline forms or right aligned forms generally aren't good on mobile.
  • Stacked forms are the most usable on any platform.
  • They're the easiest to get right from an i18n perspective.

Proposed markup for single field inputs:

<label class="elgg-input-wrapper">
  <span class="elgg-input-label">Label</span>
  <input>
  <ul>
    <li>Error messages</li>
  </ul>
</label>

And for checkboxes and radios:

<fieldset class="elgg-input-wrapper">
  <legend class="elgg-input-label">Label</legend>
  <ul class="elgg-input-checkboxes">
    <li><label><input type="checkbox">Checkbox label</label></li>
  </ul>
</fieldset>
@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Feb 8, 2014

Member

Hmmm... I felt like I was repeating myself. We might want to consolidate all of these: #4459, #3269, #3061, #2032

Member

ewinslow commented Feb 8, 2014

Hmmm... I felt like I was repeating myself. We might want to consolidate all of these: #4459, #3269, #3061, #2032

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 8, 2014

Contributor

Well it has been a pain point for as long as I have been involved with Elgg. And it's been two years since you have opened this ticket. Not sure if 1.9+1 release will introduce ElggForm class, but if it does, we could bundle it in there. Otherwise, let's add a set of views for most common use cases. I would keep label out of input, rather add wrapper views.

Contributor

hypeJunction commented Feb 8, 2014

Well it has been a pain point for as long as I have been involved with Elgg. And it's been two years since you have opened this ticket. Not sure if 1.9+1 release will introduce ElggForm class, but if it does, we could bundle it in there. Otherwise, let's add a set of views for most common use cases. I would keep label out of input, rather add wrapper views.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 8, 2014

Contributor

I am not sure I am in favour of nesting an input within a label. Plus that would require far more work to migrate existing forms.

Contributor

hypeJunction commented Feb 8, 2014

I am not sure I am in favour of nesting an input within a label. Plus that would require far more work to migrate existing forms.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Feb 9, 2014

Member

Wrapping an input within a label is a very standard way of associating the label with the input without using for and ID attributes. Nothing to be scared of here. :)

Since most of our forms do not associate inputs with labels we would have to revisit basically all of them anyways. Also, if we're serious about using angular for the front end we will have to rewrite our forms in plain HTML and Javascript instead of PHP.

Member

ewinslow commented Feb 9, 2014

Wrapping an input within a label is a very standard way of associating the label with the input without using for and ID attributes. Nothing to be scared of here. :)

Since most of our forms do not associate inputs with labels we would have to revisit basically all of them anyways. Also, if we're serious about using angular for the front end we will have to rewrite our forms in plain HTML and Javascript instead of PHP.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Feb 9, 2014

Member

ElggForm will never happen. Direct HTML is always going to be better, especially with Angular.

Member

ewinslow commented Feb 9, 2014

ElggForm will never happen. Direct HTML is always going to be better, especially with Angular.

@hellekin

This comment has been minimized.

Show comment
Hide comment
@hellekin

hellekin Feb 9, 2014

I would not embed the error messages within the <label>, and would keep error messages consistent. Also, using id attributes on the wrapper elements would be helpful for styling. And <label> itself probably does not need a class.

<div id="some-text" class="elgg-form-input elgg-form-field">
  <label>
    <span>Label</span>
    <input name="some-text" type="text">
  </label>
  <!-- optional --><p class="elgg-input-legend">Some help</p> 
  <ul>
    <li class="elgg-notice">Error message</li>
    <li class="elgg-warning">Other error message</li>
  </ul>
</div>

For radio buttons:

<fieldset id="radio-name" class="elgg-form-radio elgg-form-field">
  <!-- optional --><legend>Some help</legend> 
  <label><input type="radio" name="radio-name" value="1"><span>choice 1</span></label>
  <label><input type="radio" name="radio-name" value="2"><span>choice 2</span></label>
  <ul>
    <li class="elgg-notice">Error message</li>
    <li class="elgg-warning">Other error message</li>
  </ul>
</div>

Ditto for checkboxes.

CSS would look like:

.elgg-form-field legend, .elgg-input-legend { ... } /* for legend / usage tips */
.elgg-form-field > ul { ... } /* for field errors */
.elgg-form-field label, .elgg-input-label { ... } /* for labels */
.elgg-form-input > label { ... } /* for input-specific labels */
.elgg-form-radio > label { ... } /* for radio-specific labels */
/* various levels of errors */
.elgg-notice { ... }
.elgg-warning { ... }
.elgg-failure { ... }
.elgg-success { ... }
...

hellekin commented Feb 9, 2014

I would not embed the error messages within the <label>, and would keep error messages consistent. Also, using id attributes on the wrapper elements would be helpful for styling. And <label> itself probably does not need a class.

<div id="some-text" class="elgg-form-input elgg-form-field">
  <label>
    <span>Label</span>
    <input name="some-text" type="text">
  </label>
  <!-- optional --><p class="elgg-input-legend">Some help</p> 
  <ul>
    <li class="elgg-notice">Error message</li>
    <li class="elgg-warning">Other error message</li>
  </ul>
</div>

For radio buttons:

<fieldset id="radio-name" class="elgg-form-radio elgg-form-field">
  <!-- optional --><legend>Some help</legend> 
  <label><input type="radio" name="radio-name" value="1"><span>choice 1</span></label>
  <label><input type="radio" name="radio-name" value="2"><span>choice 2</span></label>
  <ul>
    <li class="elgg-notice">Error message</li>
    <li class="elgg-warning">Other error message</li>
  </ul>
</div>

Ditto for checkboxes.

CSS would look like:

.elgg-form-field legend, .elgg-input-legend { ... } /* for legend / usage tips */
.elgg-form-field > ul { ... } /* for field errors */
.elgg-form-field label, .elgg-input-label { ... } /* for labels */
.elgg-form-input > label { ... } /* for input-specific labels */
.elgg-form-radio > label { ... } /* for radio-specific labels */
/* various levels of errors */
.elgg-notice { ... }
.elgg-warning { ... }
.elgg-failure { ... }
.elgg-success { ... }
...
@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Feb 9, 2014

Member

I like elgg-form-field much better than elgg-input-wrapper in terms of
naming.

The problem I have with removing classes is that it ties visual
presentation to semantics too closely. I know it feels redundant in some
cases but then as soon as you want the same visual presentation on
semantically different HTML, it falls apart.

Why not put the error messages inside the label? Well have to test with a
screen reader to see what difference this makes, if any.

I really just don't want to tie visual presentation to semantics. I don't
want to force people into a structure that doesn't work for them.
On Feb 8, 2014 10:01 PM, "hellekin" notifications@github.com wrote:

I would not embed the error messages within the , and would keep
error messages consistent. Also, using id attributes on the wrapper
elements would be helpful for styling. And itself probably does
not need a class.

Label

Some help

  • Error message
  • Other error message

For radio buttons:

Some help choice 1 choice 2
  • Error message
  • Other error message

Ditto for checkboxes.

CSS would look like:

.elgg-form-field legend, .elgg-input-legend { ... } /* for legend / usage tips /.elgg-form-field > ul { ... } / for field errors /.elgg-form-field label, .elgg-input-label { ... } / for labels /.elgg-form-input > label { ... } / for input-specific labels /.elgg-form-radio > label { ... } / for radio-specific labels // various levels of errors */.elgg-notice { ... }.elgg-warning { ... }.elgg-failure { ... }.elgg-success { ... }...


Reply to this email directly or view it on GitHubhttps://github.com/Elgg/Elgg/issues/6356#issuecomment-34566517
.

Member

ewinslow commented Feb 9, 2014

I like elgg-form-field much better than elgg-input-wrapper in terms of
naming.

The problem I have with removing classes is that it ties visual
presentation to semantics too closely. I know it feels redundant in some
cases but then as soon as you want the same visual presentation on
semantically different HTML, it falls apart.

Why not put the error messages inside the label? Well have to test with a
screen reader to see what difference this makes, if any.

I really just don't want to tie visual presentation to semantics. I don't
want to force people into a structure that doesn't work for them.
On Feb 8, 2014 10:01 PM, "hellekin" notifications@github.com wrote:

I would not embed the error messages within the , and would keep
error messages consistent. Also, using id attributes on the wrapper
elements would be helpful for styling. And itself probably does
not need a class.

Label

Some help

  • Error message
  • Other error message

For radio buttons:

Some help choice 1 choice 2
  • Error message
  • Other error message

Ditto for checkboxes.

CSS would look like:

.elgg-form-field legend, .elgg-input-legend { ... } /* for legend / usage tips /.elgg-form-field > ul { ... } / for field errors /.elgg-form-field label, .elgg-input-label { ... } / for labels /.elgg-form-input > label { ... } / for input-specific labels /.elgg-form-radio > label { ... } / for radio-specific labels // various levels of errors */.elgg-notice { ... }.elgg-warning { ... }.elgg-failure { ... }.elgg-success { ... }...


Reply to this email directly or view it on GitHubhttps://github.com/Elgg/Elgg/issues/6356#issuecomment-34566517
.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Oct 20, 2014

Contributor

Is there a milestone for this?
Perhaps this could be one of the tasks for the plugin rewrite that will demonstrate all the best practices?

Contributor

hypeJunction commented Oct 20, 2014

Is there a milestone for this?
Perhaps this could be one of the tasks for the plugin rewrite that will demonstrate all the best practices?

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Oct 20, 2014

Member

No milestone. I have been creating a few milestones for tracking larger efforts that will require multiple PRs. This could go into a new "Revamp forms" milestone

Member

ewinslow commented Oct 20, 2014

No milestone. I have been creating a few milestones for tracking larger efforts that will require multiple PRs. This could go into a new "Revamp forms" milestone

@ewinslow ewinslow added this to the Revamp forms milestone Oct 20, 2014

@hellekin

This comment has been minimized.

Show comment
Hide comment
@hellekin

hellekin Jan 29, 2015

Sorry for the delay. No @ewinslow, removing the class doesn't change anything besides the addressing path to the element (instead of to the class).

Dissociating error messages from the label allows more control over how the message shows. With absolute positioning you can still "tie" the message to the label, but if you prefer having error messages independent from the label (e.g. on a line by themselves or on the top bar), you can as well much more easily (if at all).

Sorry for the delay. No @ewinslow, removing the class doesn't change anything besides the addressing path to the element (instead of to the class).

Dissociating error messages from the label allows more control over how the message shows. With absolute positioning you can still "tie" the message to the label, but if you prefer having error messages independent from the label (e.g. on a line by themselves or on the top bar), you can as well much more easily (if at all).

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Oct 29, 2015

Contributor

I have been doing a lot of work with forms and validation lately. I am packaging everything up into a bunch of smaller plugins. First one is here that standardizes the markup: https://github.com/hypeJunction/elgg_forms.

Main motivation is have a view for everything so it's easy to extend and replace. There were some themes that needed the markup to into 3 columns, so it was a hassle with large views.

Contributor

hypeJunction commented Oct 29, 2015

I have been doing a lot of work with forms and validation lately. I am packaging everything up into a bunch of smaller plugins. First one is here that standardizes the markup: https://github.com/hypeJunction/elgg_forms.

Main motivation is have a view for everything so it's easy to extend and replace. There were some themes that needed the markup to into 3 columns, so it was a hassle with large views.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 29, 2015

Member

@hypeJunction looks awesome. 👍 for adding the views and elgg_view_input() to core pretty much as is.

Member

mrclay commented Oct 29, 2015

@hypeJunction looks awesome. 👍 for adding the views and elgg_view_input() to core pretty much as is.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Oct 29, 2015

Contributor

Ok, I will wrap it up into a PR soon.

Contributor

hypeJunction commented Oct 29, 2015

Ok, I will wrap it up into a PR soon.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Oct 30, 2015

Contributor

PR: #9086

Contributor

hypeJunction commented Oct 30, 2015

PR: #9086

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment