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

#42 fix for checkbox, file, and radio input focus #51

Merged
merged 5 commits into from
Jun 4, 2013

Conversation

codepb
Copy link
Contributor

@codepb codepb commented Jun 1, 2013

Added styles for checkbox, file, and radio input focus as they were not
affected by borders.

Tested in IE7,8,9,10 and Chrome

Added styles for checkbox, file, and radio input focus as they were not
affected by borders.
Added a style so .pure-menu-separator will also work in vertical menu
and not just horizontal.
@codepb
Copy link
Contributor Author

codepb commented Jun 2, 2013

Added a fix for the pure-menu-separator so it will display in horizontal menus to address issue #53. Fixed with commit 82281b5.

Tested in IE7,8,9,10 and Chrome.

made sure sub menus, the separator is displayed horizontally again
@@ -109,3 +109,17 @@
padding-right:30px;
}

/* Adjusting separator for vertical menus */
.pure-menu-horizontal li.pure-menu-separator {
height: 20px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feel brittle; we should see if there's a more bullet-proof way to make this expand to fill the height of the container and works reasonable across browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated to what might possibly be a better solution in 8cb7efc. Now sets a height on the menu of 2.4em, and then a 50% height on the separator. Tested in Chrome and IE7+

@ericf
Copy link
Collaborator

ericf commented Jun 3, 2013

@codepb looks good, thanks! I think it's worth us investigating a better way to make the vertical separator fill its container than using a hard-coded height: 20px as noted above.

@codepb
Copy link
Contributor Author

codepb commented Jun 3, 2013

@ericf no problem. I agree with the hard-coded height being brittle. I couldn't find a responsive solution to fill 100% so fell back on the 20px separator. I believe the menus would require a bit of a rework to be able to do that but someone may have a better idea that I haven't thought about.

Changed to allow for relative measurements for the separator.
@codepb
Copy link
Contributor Author

codepb commented Jun 4, 2013

@ericf I believe I have made a more robust solution now. It uses em to set a menu height, and a percentage height on separator.

.pure-menu-horizontal li.pure-menu-separator {
height: 50%;
width: 1px;
margin: 2px 7px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should now remove the top/bottom margin since you now have height: 50%.

@tilomitra here's another place where the values and units (7px) don't make sense to me. I think this should move to 0.5em which would translate to 8px, which then makes sense for a base font size of 16px.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericf Addressed the top/bottom margin in commit ed63678

@ericf
Copy link
Collaborator

ericf commented Jun 4, 2013

@codepb great idea, I think this is much better! :shipit:

@tilomitra as part of a second pass, I think we need to clean up the pixel values used to something that's more consistent and fits in better with the base 16px font size, like I've said above and elsewhere. But let's do that in a separate pass and not hold up this PR.

Removed the top and bottom margin from the separator in the horizontal
menu.
.pure-menu-horizontal li li.pure-menu-separator {
height: 1px;
width: auto;
margin: 7px 2px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use ems instead of px to be consistent?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do this in another pass, there's px all over the place in this file.

@tilomitra
Copy link
Contributor

@ericf Ok I can go through and clean up the px/em stuff later. Let's pull this in.

@ericf
Copy link
Collaborator

ericf commented Jun 4, 2013

@codepb Merging this in now. I will add the HISTORY.md entries for this stuff as well. Thanks so much! In the future it will probably be easier if you end up creating feature/topic branches, that way this could have been split into two PRs and be isolated from your master branch.

@codepb
Copy link
Contributor Author

codepb commented Jun 4, 2013

@ericf no problem, I realised that was easier after I started doing the work! Thanks for the advice.

ericf added a commit to ericf/pure that referenced this pull request Jun 4, 2013
@ericf ericf merged commit ed63678 into pure-css:master Jun 4, 2013
@AurelioDeRosa AurelioDeRosa mentioned this pull request Jun 13, 2013
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.

None yet

5 participants