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

Allow preview controller to be customized via config options #493

Merged

Conversation

swanson
Copy link
Contributor

@swanson swanson commented Oct 1, 2020

Resolves #434

Summary

Adds a preview_base_controller configuration option to allow users to override the base controller used for previews.

@swanson
Copy link
Contributor Author

swanson commented Oct 1, 2020

I'm up for trying to add tests case if desired. I tried for a bit but was having problems:

  • I used the same approach as the with_preview_route in the test helpers but it didn't seem to pick up the configuration changes I was making
  • I didn't have any great ideas on what to assert as the original issue did not have any specific use-cases on what kinds of customization would be expected

Open to ideas / feedback

Copy link
Collaborator

@juanmanuelramallo juanmanuelramallo left a comment

Choose a reason for hiding this comment

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

Maybe we can do something like this https://github.com/github/view_component/blob/master/test/test_helper.rb#L24 to test this feature?

lib/view_component/base.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Juan Manuel Ramallo <juanmanuelramallo@hey.com>
@swanson

This comment has been minimized.

test/test_helper.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@juanmanuelramallo juanmanuelramallo left a comment

Choose a reason for hiding this comment

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

I'm not sure how @joelhawksley would like to proceed. IMHO we can extract the failing test out of this PR so that we can find a good way to test it.

@swanson

This comment has been minimized.

@swanson swanson changed the title Allow parent controller to be customized via config options Allow preview controller to be customized via config options Oct 1, 2020
@swanson

This comment has been minimized.

Copy link
Collaborator

@juanmanuelramallo juanmanuelramallo left a comment

Choose a reason for hiding this comment

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

Hey I think I found the issue why the test helper method you added wasn't working. For some reason this variable https://github.com/github/view_component/pull/493/files#diff-28535a42af4bc1dff3bc550d0e400775R91 always contains "view_components". So I tried moving that inside the route definition block, and it worked as expected.

It might be related to the fact that the rails app has already been initialized hence the variable remains the same.

However, when we place that variable inside the route definition block (I mean here https://github.com/github/view_component/pull/493/files#diff-28535a42af4bc1dff3bc550d0e400775R93) and the test helper reloads the app that block is re-run. Ending up in the result we expect.

Does this make sense?

test/app/controllers/my_preview_controller.rb Outdated Show resolved Hide resolved
@swanson
Copy link
Contributor Author

swanson commented Oct 4, 2020

@juanmanuelramallo Wow! That was tricky, I don't think I would ever have thought to test that, thanks!

Copy link
Collaborator

@juanmanuelramallo juanmanuelramallo left a comment

Choose a reason for hiding this comment

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

Neat!

@BlakeWilliams
Copy link
Contributor

@swanson This looks great! Can you add a changelog entry for this improvement?

once that's complete, @juanmanuelramallo would you like to merge this PR?

@juanmanuelramallo
Copy link
Collaborator

@BlakeWilliams sure thing! Is there anything in particular I should have in mind? (it'd be my first time merging a PR here)

@BlakeWilliams
Copy link
Contributor

@juanmanuelramallo I generally follow the steps listed here, https://github.com/github/view_component/blob/master/CONTRIBUTING.md#submitting-a-pull-request

Things like ensuring that folks add themselves as a contributor, changelog entries if applicable, etc.

@juanmanuelramallo
Copy link
Collaborator

Noted, thanks!

CHANGELOG.md Outdated Show resolved Hide resolved
@juanmanuelramallo
Copy link
Collaborator

Hey @swanson sorry for the back and forth, but would you mind adding yourself in the list of contributors at the bottom of the readme?

Step 6 in this guide https://github.com/github/view_component/blob/master/CONTRIBUTING.md#submitting-a-pull-request and don't forget about the first part of step 8 🙈

@swanson
Copy link
Contributor Author

swanson commented Oct 6, 2020

@juanmanuelramallo no problem, done!

@juanmanuelramallo juanmanuelramallo merged commit e80bb3e into ViewComponent:master Oct 6, 2020
@juanmanuelramallo
Copy link
Collaborator

Thank you @swanson!

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.

Make previews controller extensible
4 participants