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 new helper API #1860

Merged
merged 37 commits into from
Nov 3, 2023
Merged

Conversation

reeganviljoen
Copy link
Collaborator

@reeganviljoen reeganviljoen commented Sep 25, 2023

closes #1848

What are you trying to accomplish?

as discussed I am attempting to add a more explicit helpers api to replace helper.<helper>

What approach did you choose and why?

I used a active support concern to add the method to make it modular and optional

Anything you want to highlight for special attention from reviewers?

@reeganviljoen
Copy link
Collaborator Author

@camertron I haven't added the test helpers discussed yet, but can you take a look at what is added so far

Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

This is looking good, thanks @reeganviljoen! Could I ask you to take a look at my comments and write some docs as well?

docs/CHANGELOG.md Outdated Show resolved Hide resolved
lib/view_component/helpers_api.rb Outdated Show resolved Hide resolved
test/sandbox/app/components/helpers_api_component.rb Outdated Show resolved Hide resolved
test/sandbox/test/rendering_test.rb Outdated Show resolved Hide resolved
@Spone
Copy link
Collaborator

Spone commented Oct 1, 2023

Thanks for this @reeganviljoen! Let us know if you need help with the docs!

@reeganviljoen reeganviljoen requested a review from Spone as a code owner October 23, 2023 20:05
@reeganviljoen
Copy link
Collaborator Author

@camertron, @Spone I got the use_helpers helper top work finally after many hours of debugging strange errors, however I believe it can be further refactored quite a bit but due to these errors whenever I try it breaks, if you can possibly help(if you can) it would be much appreciated

@reeganviljoen reeganviljoen self-assigned this Oct 24, 2023
Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

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

Looking good! I had a few suggestions/thoughts but I like where this is going!

lib/view_component/test_helpers.rb Outdated Show resolved Hide resolved
lib/view_component/use_helpers.rb Outdated Show resolved Hide resolved
# Set the controller helpers to be used while executing the given block,
#
# ```ruby
# with_helpers(UsersHelper) do
Copy link
Contributor

Choose a reason for hiding this comment

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

I was imagining that this could be used to mock helpers instead of including helpers that should already be present on the view context.

e.g.

test "foo" do
  # delegates to the current_user method
  with_helpers(current_user: :current_user) do
    render_inline MyComponent.new
  end

  current_user = create(:user)
  # stubs the current_user helper to return this value
  with_helpers(current_user: current_user) do
    render_inline MyComponent.new
  end
end

def current_user
  # some logic
end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@BlakeWilliams moving this to a new pr

lib/view_component/use_helpers.rb Outdated Show resolved Hide resolved
lib/view_component/use_helpers.rb Outdated Show resolved Hide resolved
@reeganviljoen
Copy link
Collaborator Author

@Spone, @BlakeWilliams , @camertron I have decided to separate the test_helper portion of this pull request as their seems to be a few unrelated issues that would benefit from focused attention, would any of you be able to review what is left of the use_helpers method, I will be adding docs soon

docs/CHANGELOG.md Outdated Show resolved Hide resolved
docs/guide/helpers.md Outdated Show resolved Hide resolved
docs/guide/helpers.md Outdated Show resolved Hide resolved
docs/guide/helpers.md Outdated Show resolved Hide resolved
@reeganviljoen reeganviljoen requested a review from Spone October 31, 2023 20:22
@Spone
Copy link
Collaborator

Spone commented Oct 31, 2023

@Spone, @BlakeWilliams , @camertron I have decided to separate the test_helper portion of this pull request

Good call, let's focus on the core feature, then add the test helper.

Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Awesome, I think this is ready to go!

@camertron camertron enabled auto-merge (squash) November 3, 2023 18:18
@camertron camertron merged commit 2bc5763 into ViewComponent:main Nov 3, 2023
23 checks passed
claudiob pushed a commit to claudiob/view_component that referenced this pull request Dec 22, 2023
claudiob pushed a commit to claudiob/view_component that referenced this pull request Jan 3, 2024
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.

Replace helpers with explicit api in v4
4 participants