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

Replace button_link_to with either button_to or link_to #2601

Merged
merged 3 commits into from
Feb 26, 2018

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Feb 23, 2018

Similar to #2600 (and built on top of it to avoid conflicts)

This replaces all uses of our button_link_to helper with either button_to or link_to.

There's no reason for button_link_to to exist. Either we want a link (which we can style like a button easily if we want) or just a button. We should explicitly use the one we want given the situation.

All occurrences of button_link_to can be replaced with either button_to or link_to with class: 'btn btn-primary'. Both Rails built-in helpers can have a :method specified, and both are able to perform a GET request.

I've chosen to replace all button_link_to with a method with button_to, since that was the behaviour of the method, and the most appropriate use of a button and form tag.

I've replaced all button_link_to which did not specify a method (a GET request is desired) with link_to, since that was the previous behaviour and because this is what a link is.

(This should be rebased before merging)

@jhawthorn jhawthorn added the changelog:solidus_backend Changes to the solidus_backend gem label Feb 23, 2018
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thank you. That’s over due.

I appreciate the sensible distinction between links and buttons :ok-hand:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants