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 govuk-equilateral-height function usage in shape-arrow helper #766

Merged
merged 3 commits into from
Jun 6, 2018

Conversation

kr8n3r
Copy link

@kr8n3r kr8n3r commented Jun 5, 2018

In #762 we changed the name of the govuk-equilateral-height function to _govuk-equilateral-height to signify that is is a private function.

Unfortunately we forgot to update the name where it was used to create the open/closed indicator for the details component.

This PR fixes this.

Before:
screen shot 2018-06-05 at 19 39 00

After:
screen shot 2018-06-05 at 19 39 41

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-766 June 5, 2018 18:19 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-766 June 5, 2018 18:21 Inactive
@kr8n3r kr8n3r changed the title Fix govuk-equilateral-height function usage in details component Fix govuk-equilateral-height function usage in shape-arrow helper Jun 5, 2018
@kr8n3r kr8n3r force-pushed the fix-broken-details-triangle branch from d1db3d1 to fea49fe Compare June 5, 2018 18:42
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-766 June 5, 2018 18:42 Inactive
@36degrees
Copy link
Contributor

Argh, good catch - thanks for that. I think I understand why, but it's pretty annoying that Sass silently outputs the function call verbatim. I wonder if there's a way we could try and catch that if it happens.

@kr8n3r
Copy link
Author

kr8n3r commented Jun 6, 2018

i guess only if we wrap it in a custom mixin that check if function exist using sass's built-in function-exists

alternatively, if we use get-function(myfunc) when calling a function, that will throw an error in compilation

@36degrees 36degrees force-pushed the fix-broken-details-triangle branch 2 times, most recently from 349e298 to 1e14b05 Compare June 6, 2018 10:22
@36degrees
Copy link
Contributor

Added a test that greps the CSS for anything matching /govuk-[\w-]+\(.*?\)/g, which should catch most function calls that slip through the net… unless they misspell govuk…

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-766 June 6, 2018 10:47 Inactive
@kr8n3r kr8n3r force-pushed the fix-broken-details-triangle branch from 1e14b05 to 0817fb9 Compare June 6, 2018 10:57
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-766 June 6, 2018 10:58 Inactive
@kr8n3r
Copy link
Author

kr8n3r commented Jun 6, 2018

fixed the extra semicolon in the test

Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

👍 Tested with the details component and mobile navigation menu button.

@36degrees 36degrees force-pushed the fix-broken-details-triangle branch from 0817fb9 to 20877c0 Compare June 6, 2018 14:21
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-766 June 6, 2018 14:21 Inactive
@36degrees
Copy link
Contributor

Added a big ole' comment around what the test is looking for

@36degrees 36degrees force-pushed the fix-broken-details-triangle branch from 20877c0 to b684f5a Compare June 6, 2018 14:22
@kr8n3r kr8n3r merged commit 8233093 into master Jun 6, 2018
@kr8n3r kr8n3r deleted the fix-broken-details-triangle branch June 6, 2018 14:40
@hannalaakso hannalaakso mentioned this pull request Jun 14, 2018
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.

4 participants