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

[WIP] Remove instance variables #243

Closed
wants to merge 3 commits into from

Conversation

jensljungblad
Copy link
Contributor

This PR attempts to solve #220.

Started looking at this and it will be tricky, but I like the concept. First I replaced all instance variables with helper methods, which I got working. Unfortunately our code is a bit hard to follow. The service object is modified during the request cycle by different parts of the code, so things need to happen in a particular order for things to work, which is why we set the instance variables in the order that we do. Not nice, we should try and fix that as well.

Then I tried replacing the helper methods with locals, which is what we want. That way, variables aren't automatically available to all views and partials, but have to be explicitly passed, which is nice. However, we have a problem with some of our view helpers, such as the translation and form helpers. They require access to resource_class and resource_parents. We could pass them to the translation helper, but it's trickier with the form helper which mimics the Rails helper.

Do we want to expose some things as helper methods, available everywhere and some things passed down as locals?

@jensljungblad
Copy link
Contributor Author

We could leave instance variables in the controller layer, but send locals to the view layer, if that makes it easier.

For the translation helper, pass it the needed locals.

For the form helper, perhaps we can use Rails built in method of passing resource parents..?

@jensljungblad jensljungblad deleted the remove-instance-variables branch February 15, 2018 14:48
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.

1 participant