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

Add basic govuk_link_to helper and test #15

Merged
merged 3 commits into from
May 18, 2020
Merged

Conversation

peteryates
Copy link
Member

@peteryates peteryates commented May 17, 2020

This is a bit of an experiment related to the suggestion made in the comments of #3, it adds a app/helpers directory and an initial #govuk_link_to helper that's a wrapper of Rails' #link_to

  • Add a proof of concept helper

  • Ensure it works with the dummy app
    Screenshot from 2020-05-17 12-33-49

  • Add a test (and make it pass!)

Testing currently doesn't work

  • in the submitted state we get undefined method link_to'`
  • adding include ActionView::Helpers::UrlHelper to the top of govuk_link_helper_spec.rb or spec_helper.rb makes the individual file pass but when running the suite we get cyclic include detected

Not quite sure how to make it play ball with RSpec.

@peteryates peteryates added the help wanted Extra attention is needed label May 17, 2020
@aliuk2012 aliuk2012 assigned aliuk2012 and unassigned aliuk2012 May 18, 2020
This results in a 'cyclic include detected' error if the include is
outside the describe block. Thanks @rjlynch for solving this!
@peteryates peteryates removed the help wanted Extra attention is needed label May 18, 2020
Copy link
Contributor

@rjlynch rjlynch left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me

@aliuk2012
Copy link
Contributor

Can you pass custom css classes?

@peteryates
Copy link
Member Author

Just finishing off that functionality @aliuk2012, ensuring it works for strings and arrays

@peteryates
Copy link
Member Author

@aliuk2012 so yeah, having had a think any custom classes will override the default. Ensuring safe merging made it a bit too complex. We can revisit this behaviour if needs be

@peteryates peteryates merged commit 7819303 into master May 18, 2020
@peteryates peteryates deleted the add-url-helpers branch May 18, 2020 12:51
@peteryates peteryates mentioned this pull request May 18, 2020
21 tasks
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

3 participants