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

Indent report a problem toggle and form correctly #1057

Merged
merged 1 commit into from Jun 8, 2017
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented May 23, 2017

When on mobile, in some layouts the report a problem link and form get indented twice – once because of a 15px margin applied to #wrapper, and once again on report a problem itself.

  • If the link is nested within wrapper, don’t indent again
  • Keeps indentation if no wrapper is present

The homepage is a special case, it uses #wrapper but removes the margin

  • Include site-width-container again with higher specificity for the homepage. This prevents the 0 margin resets from taking effect.

Before

screen shot 2017-05-23 at 13 38 21

After

screen shot 2017-05-23 at 13 38 07

Previous attempt to fix: #660 reverted in #662

Fixes #998

@fofr
Copy link
Contributor Author

@fofr fofr commented May 23, 2017

Homepage

screen shot 2017-05-23 at 13 45 48
screen shot 2017-05-23 at 13 45 57

@fofr
Copy link
Contributor Author

@fofr fofr commented May 23, 2017

Tested by deploying to integration and looking at Whitehall, Homepage, Frontend and government-frontend layouts. Also searched for other #wrapper resets but found none.

@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented May 23, 2017

The problem here is that the component should not define a width or horizontal margin, it should sit inside the parent wrapper.

I'd prefer to leave this as is and figure out what the work is to have this component sit within the pages' wrapper since this seems like it could break

@fofr
Copy link
Contributor Author

@fofr fofr commented Jun 5, 2017

@nickcolley Using the site on mobile, this bug continues to bother me. A "done properly" fix is complex and requires upgrading to the wrapper layout on our older pages, which I think will happen organically but over a long time.

A more complex fix is also more likely to break, or take more time testing/developing.

Copy link
Contributor

@andysellick andysellick left a comment

I agree that fixing this properly is a wider issue and that it would be better to look into that, but I also think the current behaviour looks quite broken and this change fixes that. Lacking the time to look into a better fix, I think this works.

@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Jun 8, 2017

I'd be okay merging if there was a TODO with a basic description of why this hack exists 👍

When on mobile, in some layouts the report a problem link and form get
indented twice – once because of a 15px margin applied to #wrapper, and
once again on report a problem itself.

* If the link is nested within wrapper, don’t indent again
* Keeps indentation if no wrapper is present

The homepage is a special case, it uses #wrapper but removes the margin

* Include site-width-container again with higher specificity for the
  homepage. This prevents the `0` margin resets from having affect.
@fofr fofr force-pushed the fix-report-indent branch to 67af679 Jun 8, 2017
@fofr
Copy link
Contributor Author

@fofr fofr commented Jun 8, 2017

Added explanatory comment and a TODO about preferred styling.

@fofr fofr merged commit a1c9cd5 into master Jun 8, 2017
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
security/snyk No new vulnerabilities
Details
@fofr fofr deleted the fix-report-indent branch Jun 8, 2017
@benilovj
Copy link
Contributor

@benilovj benilovj commented Jun 8, 2017

🎆 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.