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

Fix spacing of checkboxes with multi-line labels #293

Closed
wants to merge 1 commit into from

Conversation

dmethvin-gov
Copy link
Contributor

Fixes #233

I don't have a convenient way to test this, I can't get to staging. I tried playing with a codesandbox but it won't let me use a commit from a GitHub branch. @rtluu can you verify that this fixes the issue?

Types of changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've reviewed the guidelines for contributing to this repository.
  • I've checked style and lint with npm run lint.
  • I built the production version with npm run build.
  • I added tests to verify a bug fix or new functionality.
  • All tests pass locally with npm test.
  • My change requires a change to the documentation, and I've updated the documentation accordingly.

@annekainicUSDS
Copy link
Contributor

In trying to test this locally, it looks like it might not have totally fixed it, but it could be getting overridden by the temporary fix the Vets.gov team put in for this issue. @rtluu can you provide a link to the fix from the Vets.gov side so I can test that interaction? Thanks!

@rtluu
Copy link

rtluu commented Oct 11, 2018

hey @annekainicUSDS, the 4th chapter of this form on staging has the multiline checkbox implementation https://staging.vets.gov/education/gi-bill-school-feedback.

screen shot 2018-10-11 at 1 56 57 pm

@annekainicUSDS
Copy link
Contributor

Sorry, I meant the lines of code that fixed this issue that were added to Vets.gov, @rtluu.

@rtluu
Copy link

rtluu commented Oct 11, 2018

Ohh gotcha, @cehsu would you be able to point Anne towards the change that was made on vets-website?

@cehsu
Copy link

cehsu commented Oct 16, 2018

Here is the multiline label implementation on Vets.gov, please let me know if anything else would be helpful.
https://github.com/department-of-veterans-affairs/vets-website/blob/5c13ca132cd7154a8ad90296e19a6934af815f8f/src/applications/edu-benefits/feedback-tool/helpers.js#L230

@annekainicUSDS
Copy link
Contributor

annekainicUSDS commented Oct 23, 2018

@cehsu @rtluu I'm confused about the latest information provided in this ticket. It seems like we're talking about two different cases of checkbox usage. In the original ticket for this issue, you included a screenshot of a page where the second set of checkboxes had extra spacing between them. You alluded to a temporary fix added on the Vets.gov side to remove this extra spacing until a permanent fix could be applied to USFS. When I navigate to that page and inspect those checkboxes, I see a style that adds margin-top: 0 to those checkboxes.

screen shot 2018-10-23 at 9 08 27 am

If that style is removed, I can see the extra space between the checkboxes. The style that should be adding the margin-top: 0 below is not being applied to the second set of checkboxes properly.

screen shot 2018-10-23 at 9 08 55 am

This first style seems like the one the Vets.gov team added to overwrite the issue. Is that correct? If so, could you please provide a link in the codebase to that style?

The additional comments in this PR that include a screenshot of checkboxes with italicized text beneath them and the link to the markdown implementation of those checkboxes seems unrelated to the original reason this issue was filed, but correct me if I'm wrong on that.

@dmethvin-gov
Copy link
Contributor Author

I rebased and force-pushed to ensure it won't try to put the built css file back when this is merged.

@annekainicUSDS
Copy link
Contributor

@rtluu @cehsu ping on this! Please see my latest comment for the information we're still looking for from you guys on this PR.

@cehsu
Copy link

cehsu commented Nov 28, 2018

Thanks @annekainicUSDS, they are provided here.

@annekainicUSDS
Copy link
Contributor

@dmethvin-gov @cehsu @rtluu I've dug into this issue and PR quite a bit, and I think this particular fix is not the right solution for the problem. Here are the things I've learned:

  • The CSS fix in this PR does not remove the margin between the checkboxes are intended. That's because it's targeting classes with a parent that has the class .schemaform-first-field, which is only wrapping the first checkbox in a list of checkboxes. Therefore the margin-top: 0 will not be applied to subsequent checkboxes.
  • For the specific page in the feedback tool we've been looking at, there are 2 distinct groups of checkboxes. For some reason, the first group has the class .schemaform-first-field added at the top level of the markup (on the <fieldset/>), whereas the second group only has the .schemaform-first-field class added to the first checkbox in the group, which is why the CSS is not being applied properly. I think this class .schemaform-first-field, which is applied here, is only being applied to the top of the first group of checkboxes and not the second because it finds the first instance of .schemaform-field-template and just applies the class to that first instance.
  • Regardless of the weird difference between the markup, we could add additional CSS to remove margin-top for all checkboxes, but this change will affect all checkbox groups built with USFS, and that would not be an appropriate change to make. We would want to evaluate how we want any checkbox to appear for any form generated by USFS before making that type of change.

If the Vets.gov team wants to collapse all margin between checkboxes, I believe that is a change they should keep on their side, because I don't think that's a change we want to implement for all checkbox groups in USFS.

In digging into this issue, I've discovered some inconsistencies across various form examples of how checkbox groups are rendered by USFS that I think may need investigation as a separate issue. But I think this PR solving this particular problem should be closed. Let me know if you have comments or additional feedback, otherwise I will close on Monday.

@dmethvin-gov
Copy link
Contributor Author

I'm fine with closing this, especially since I wasn't able to confirm the problem myself.

@cehsu
Copy link

cehsu commented Dec 10, 2018

Thanks for looking into this @annekainicUSDS, that's a helpful assessment and a good plan for moving forward.

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.

Inconsistent spacing between 1 line & 2 line checkboxes
4 participants