Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style improvement of radio form elements #12736

Merged
merged 1 commit into from
Oct 27, 2018
Merged

Style improvement of radio form elements #12736

merged 1 commit into from
Oct 27, 2018

Conversation

calbasi
Copy link
Contributor

@calbasi calbasi commented Aug 28, 2018

Radio inputs are too close to their labels, I think this simple improvement would be useful, as commented here:
https://lab.civicrm.org/dev/core/issues/355

Radio inputs are too close to their labels, I think this simple improvement would be useful, as commented here:
https://lab.civicrm.org/dev/core/issues/355
@civibot
Copy link

civibot bot commented Aug 28, 2018

(Standard links)

@JoeMurray
Copy link
Contributor

@calbasi I like the intent here and since it's a fussy change I'm feeling emboldened to make a fussy request. In the new version shown in gitlab image, there is more space between radio button and text associated with it than between the text and the next button. Could you maybe decrease the spacing between button and its text, and/or add spacing after text before next radio button? Also, could you provide an image showing relative space between radio button and text after it that also includes other widgets for comparison, like a checkbox? Thanks!!

@eileenmcnaughton
Copy link
Contributor

@colemanw

@colemanw
Copy link
Member

IMO a better improvement would be to wrap each input+label combo in a span or a div. It really makes styling difficult when all form elements and labels are siblings, especially because when overflowing to multiple lines the label might end up on a different line than the input.

@JoeMurray
Copy link
Contributor

+1 to @colemanw 's suggestions. But I'd also like the styling itself to change as mentioned, and for there to be a review of how this change to radio button spacing will look in comparison to checkbox spacing.

@calbasi
Copy link
Contributor Author

calbasi commented Aug 29, 2018

Hi @JoeMurray, I agree with you. Here could be an improved code:

input.crm-form-radio + label, input.crm-form-checkbox + label {
    margin-left: 7px;
    margin-right: 18px;
}

I attack some images with the before/after screenshots.
captura de pantalla de 2018-08-29 14-19-17
captura de pantalla de 2018-08-29 14-18-58
captura de pantalla de 2018-08-29 14-20-32
captura de pantalla de 2018-08-29 14-20-10

And I agree too with @colemanw , a necessary improvement would be pairs of input/label have a parent warp element, like:

<div class="edit-value content">
  <div class="option">
    <input value="1" id="CIVICRM_QFID_1_2" name="gender_id" class="crm-form-radio" type="radio">
    <label for="CIVICRM_QFID_1_2">Female</label>
  </div>
  <div class="option">
    <input value="2" id="CIVICRM_QFID_2_4" name="gender_id" class="crm-form-radio" type="radio">
    <label for="CIVICRM_QFID_2_4">Male</label>
  </div>
</div>

What about these 2 changes? I can add the first to the pull request, but I don't know where html output of radio/checkboxes is generated (but I'd like to learn about it)

@seamuslee001
Copy link
Contributor

@calbasi That html output is generated from within the HTML/QuickForm code which lives in the civicrm_packages repo

@eileenmcnaughton
Copy link
Contributor

@colemanw where are you at on this?

@calbasi
Copy link
Contributor Author

calbasi commented Oct 9, 2018

Then, what do you prefer? We could apply this first improvement, and then go for the HTML commit? Or do you prefer we start to work with HTML while this is in stand by?

@eileenmcnaughton
Copy link
Contributor

@mattwire @colemanw any thoughts on how to proceed with this one?

@mattwire
Copy link
Contributor

The css change is simple so I think this should be merged before any changes to HTML, which will be more complex and should be done in a separate PR.
However, I'd like to get @vingle input on the css changes if possible? And I see this PR has not been updated with the proposed changes in #12736 (comment)

@vingle
Copy link
Contributor

vingle commented Oct 26, 2018

I second the suggestion of accepting the css as an immediate cleanup. Regarding the markup changes I'd say yes, but should we be aiming to have a styleguide for front-end form elements and then reference that?
I realise that slows this down but there is such a mix of legacy front-end Civi markup from othe years — from tables to divs & spans, to using 17% width on the .label selector to indent inputs (which then have a 17% margin-left, and which is the main reason front-end forms aren't responsive by default). If we had a single style-guide for markup (and even, perhaps, design patterns - as @michaelmcandrew has suggested - or maybe that's a parallel discussion) then we could gradually shift changes and new work in that direction and not risk a future decision around, say, select-lists, adopting a different approach. If it's just a document on Github or Lab then it could start small (ie radio buttons for now) then iterate up to buttons, fieldsets, containers, etc?

@eileenmcnaughton
Copy link
Contributor

So based on advice from @mattwire & @vingle I believe this is mergeable and I am doing so. I understand it's prefereable to also add a margin right & if that PR emerges it can also be merged.

I strongly agree that there should be documentation (in gitlab or docs ) on what we are working towards in terms of markup clean up because we don't have anything to assess these proposals against at the moment

@eileenmcnaughton eileenmcnaughton merged commit 60a1d11 into civicrm:master Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants