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

Fixing RadioButton and Checkbox styles, solves #442 #299 #415 #489 #488 #508

Closed
wants to merge 1 commit into from
Closed

Fixing RadioButton and Checkbox styles, solves #442 #299 #415 #489 #488 #508

wants to merge 1 commit into from

Conversation

tkrugg
Copy link
Member

@tkrugg tkrugg commented Feb 3, 2015

NB: This is not ready for merge. It's only here to gather some feedback.before going further.

Additionally:

  • for now, removed holodark and ios themes for Checkbox -- this is a step back for checkbox, not radio button which didn't have any in the first place. It's not hard to do, I just need to know what priority it is.
  • moved radiobutton and checkbox to svg. This definitively solves the positioning issue we were experiencing on the various browsers, while combining DOM and characters.
  • not handling rtl for now -- prior version of d-checkbox did
  • high contrast mode looks "ok" (tested on IE and FF), though it's not perfect. In high contrast mode, a native checkbox would have a white checkmark inside a black square. The HC restyling doesn't affect svg.
  • radio-button and checkbox's css are almost a 100% identical. Any ideas on how to avoid code duplication in a clean way?
  • the bootstrap theme stylesheet contains references to bootstrap specific classes to help with interop (see here and here )

checked="{{checked}}" value="{{value}}" name="{{name}}"/>
<template role="presentation">

<input class="d-reset" type="checkbox" attach-point="focusNode,valueNode"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d-reset set font, line-height and color. This looks not needed here if only padding and margin must be set. Same comment for radiobutton.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oversight on my part.

@dmandrioli
Copy link
Member

The radio button is a bit smaller than the checkbox. Adjusting the radio button size could also improve aligment with label:
image

@wkeese
Copy link
Member

wkeese commented Feb 3, 2015

The SVG fixes #442? That's nice, but IIUC the downside is that it breaks theme customizatio, i.e. you can't have different checkbox icons depending on theme, although perhaps you can control the color.

@tkrugg
Copy link
Member Author

tkrugg commented Feb 4, 2015

The SVG fixes #442?

SVG is fixing another positioning problem (reported in #408, which I forgot to mention)

  • position of the checkmark inside the square
  • position of the dark circle inside the bigger light circle in the radio-button

When the checkbox was using DOM + a unicode character as a checkmark, it could be easily affected by a lot of CSS properties such as line-height, font-size, margins, padding... It was hard to make it look the same cross-browser (1px makes a huge difference at theses sizes) and was hurting the interoperability with bootstrap.

Radiobutton is apprently much simpler an could be done only with DOM, but I wasn't satisfied with the way it looked at small sizes: the version below was implemented with DOM only in chrome.

image

Part of my work on d-icon is see how we can have a different icon pack for each theme. This should solve your problem.

@tkrugg
Copy link
Member Author

tkrugg commented Feb 4, 2015

The radio button is a bit smaller than the checkbox.

In which browser do you see it? CSS wise they really are the same size in px.

@dmandrioli
Copy link
Member

In which browser do you see it? CSS wise they really are the same size in px.

In Chrome:
image

and IE11:

image

As mentioned, this is probably related to the different behaviour of overflow:visible on checkbox and radiobutton.

@wkeese
Copy link
Member

wkeese commented Feb 4, 2015

SVG is fixing another positioning problem (reported in #408, which I forgot to mention)
...
Part of my work on d-icon is see how we can have a different icon pack for each theme. This should solve your problem.

Well, IIUC you are researching SVG, and you are a big fan of SVG, but we haven't actually decided to switch from font-icons to SVG. And even if we decide to switch, the end result won't look like what you have in this PR: you've hardcoded the icon into the widget's template, whereas ideally it should be part of the theme.

I don't have a strong opinion, but not sure it makes sense to make this change now.

@tkrugg
Copy link
Member Author

tkrugg commented Feb 4, 2015

Every single thing you said is true. i'm just exploring here... never meant to merge this, like dazzlingly stated in bold first post.

If we ever decide to make the switch, the end result will look the same though, the markup will change, but no reason the stylesheet will. Strong opinion or not, please comment with it here #434

@dmandrioli I looked at it. I found that it's the border-radius set on the svg element that hiding part of the content, it'll be fixed.

@wkeese
Copy link
Member

wkeese commented Feb 4, 2015

Ah right, sorry, you did write that. Well anyway, it's a good proof-of-concept of using SVG for these icons.

I do think it would be easy to fix the alignment issues while still using font-icons, because it shouldn't be difficult to make one character align with other characters. Actually, that should happen naturally, assuming we aren't adding CSS that messes it up.

@cjolif cjolif force-pushed the master branch 3 times, most recently from ea71478 to 93584d3 Compare February 6, 2015 09:49
- fixed labels positionning, solves #442
- fixed inconsistent outlines, solves #299 and #415
- fixed interop with bootstrap, solves #489 and #488
 * fixed positioning inside .checkbox{,-inline} and .radio{,-inline}
 * added state dependant focus
 * using bootstrap variables for sizing, in the bootstrap theme

NB: for now, removed holodark and ios themes for Checkbox. It doesn't
exist for radio button.
@wkeese
Copy link
Member

wkeese commented Jun 19, 2016

I guess this PR got forgotten. #442 #299 #415 are already fixed. #489 #488 are still open (or perhaps the problem is fixed but the tickets weren't closed).

@tkrugg tkrugg closed this Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants