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

Search box hueristic fixes #1988

Merged
merged 7 commits into from Jun 5, 2018

Conversation

Projects
None yet
3 participants
@ericronne
Contributor

ericronne commented May 31, 2018

Closes #1759

@ericronne

This comment has been minimized.

Contributor

ericronne commented May 31, 2018

Need help narrowing the gap between radio buttons and their labels (by about 6px)
image

Checkboxes have the same problem, can fix here if convenient
image

@ericronne ericronne changed the title from [WIP] Search box hueristic fixes to Search box hueristic fixes Jun 1, 2018

@ericronne

This comment has been minimized.

Contributor

ericronne commented Jun 1, 2018

Radio buttons in style guide look good with my changes …

image

But explorer search radio buttons don't …

image

Checkboxes in style guide are optically about one pixel farther away from their labels than radio buttons are to theirs, tho …

image

☝️ #helpwanted @hbillings 😭

@ericronne ericronne requested a review from hbillings Jun 1, 2018

@hbillings

This comment has been minimized.

Member

hbillings commented Jun 1, 2018

@ericronne still looking into the radio button spacing issue, but I can tell you that the checkboxes are 1px further away because their borders are 1px thinner. :-/

@ericronne

This comment has been minimized.

Contributor

ericronne commented Jun 1, 2018

Au contraire! I'm referring to the unselected states
image

@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Jun 1, 2018

I've looked in _inputs and _forms... is it possible the width difference is being inherited from USWDS?

@hbillings

This comment has been minimized.

Member

hbillings commented Jun 1, 2018

So this seems to be some bizarre kind of DOM rendering:
This butts right up to the margin on the radio button:
screen shot 2018-06-01 at 2 50 30 pm

This one sorta spaces itself out:
screen shot 2018-06-01 at 2 50 17 pm

I haven't been able to find any rules that seem to be causing this. I even tried rendering the first one as a ul like the second is, and it still inherited the wonky spacing.

@hbillings hbillings closed this Jun 1, 2018

@hbillings hbillings reopened this Jun 1, 2018

@hbillings

This comment has been minimized.

Member

hbillings commented Jun 1, 2018

Oh, and also, @ericronne, sorry about misunderstanding your question about widths -- looks like radios have a width of 1.4rem and checkboxes have a width of 2rem, so that would probably be the cause of the discrepancy.

@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Jun 1, 2018

Heather, what's the markup difference between those examples? That extra inexplicable space reminds me of the space you see when using display: inline-block. I wonder if messing with display would do anything?

@hbillings

This comment has been minimized.

Member

hbillings commented Jun 1, 2018

@tbaxter-18f I'm almost positive you're right, but I can't quite pinpoint where it needs to happen.

You can see the markup for the horizontal radios just under the search box here: http://calc.gsa.gov/
and the vertical radios here: https://calc.gsa.gov/styleguide/#radio-buttons-and-checkboxes

The biggest difference I see is the use of divs and spans in the first and ul and lis in the second, which is why I was puzzled that changing the first markup to the second didn't have an effect. Could be that something odd is cascading, I suppose...

@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Jun 1, 2018

OK, [type=checkbox]+label::before, [type=radio]+label::before is definitely setting inline-block. Just testing with display:block; float: left closes it up, but we may want to look at a flex or grid solution. Or just make the margin less, since we know inline-block has a predictable-ish spacing.

However they both have inline-block so that's at least set the same.

Also seeing: the horizontals there are picking up a 0.75 right margin from _inputs.scss that the horizontals don't seem to be getting. But I don't think that's it.

My current theory is that it has to do with how oversized elements overflow inline elements vs block or inline-block. It looks like the horizontal ones are slipping a bit left of the parent. I wonder if the wrapping spans had were block and floated if would matter.

margin-top: 0;
}
.search-header h2 {

This comment has been minimized.

@hbillings

hbillings Jun 4, 2018

Member

These can be combined into a nested rule for clarity:

.search-header {
  p {...}
  h2 {...}
}
{' '}
(For example: Engineer, Consultant)
</p>
</div>

This comment has been minimized.

@hbillings

hbillings Jun 4, 2018

Member

We'll need to update the Jest snapshot of this file to accommodate for the markup change. You can do that by running docker-compose run app jest --updateSnapshot and committing the file that is changed as a result.

This comment has been minimized.

@ericronne

ericronne Jun 4, 2018

Contributor

okedoke

This comment has been minimized.

@ericronne

ericronne Jun 4, 2018

Contributor

ran it, but the file didn't seem to change :/

This comment has been minimized.

@hbillings

hbillings Jun 4, 2018

Member

Oops, actually, it looks like there is no snapshot for app.jsx.

@hbillings

This comment has been minimized.

Member

hbillings commented Jun 4, 2018

The bizarre thing here is that the problem persists even when the markup is the same and any custom CSS is removed:
screen shot 2018-06-04 at 1 07 05 pm

oy

}
h2 {
margin-top: $space-8x;
}

This comment has been minimized.

@hbillings

hbillings Jun 4, 2018

Member

This is making me wonder why we don't have generic heading + p rules. Those would probably be pretty handy.

This comment has been minimized.

@ericronne

ericronne Jun 5, 2018

Contributor

Great minds …

@hbillings

Looks good, though this seems to mostly be about small tweaks and not the sort of heuristic fixes we've mostly been talking about -- does the title of the issue still apply?

@ericronne

This comment has been minimized.

Contributor

ericronne commented Jun 5, 2018

The lion's share of issues in the epic cover hueristics, and this one seems to fit, since radio buttons were all running together

@ericronne

This comment has been minimized.

Contributor

ericronne commented Jun 5, 2018

After running ./docker-update.sh, here's how the radios look, compared to uswds …
image

Oddly, our spacing renders as 5(?) pixels less than uswds, despite the fact that both have a right margin of 14px. Where should i make the adjustment?

@hbillings

This comment has been minimized.

Member

hbillings commented Jun 5, 2018

@ericronne The inspector says this is happening on line 13 of components/_inputs.scss

@@ -10,7 +10,7 @@ input[type="radio"] + label,
input[type="checkbox"] + label {
margin-top: 0;
&:before {
margin-right: .6rem;
margin-right: 1rem;

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 5, 2018

Member

I wonder, now that we've figured out the spacing weirdness, what if we had no rule/override here at all and just went with the USWDS 0.75em default?

@ericronne ericronne merged commit 4139622 into develop Jun 5, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate All good!
Details

@hbillings hbillings deleted the 1759-explorer-search-box-improvements branch Sep 29, 2018

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