Skip to content

Component with translations#74

Merged
joelhawksley merged 2 commits intoViewComponent:masterfrom
juanmanuelramallo:component-with-translations
Oct 18, 2019
Merged

Component with translations#74
joelhawksley merged 2 commits intoViewComponent:masterfrom
juanmanuelramallo:component-with-translations

Conversation

@juanmanuelramallo
Copy link
Contributor

@juanmanuelramallo juanmanuelramallo commented Oct 17, 2019

Solves issue #49

  • It was failing with the following error
Error:
ActionView::ComponentTest#test_component_with_translations:
RuntimeError: Cannot use t(".title") shortcut because path is not available
  • Updated the ActionView::Component::Base object to include a virtual_path
  • The virtual path is calculated using the source location of the instance's class

- Proposing a solution for the shortcut issue #49
@juanmanuelramallo juanmanuelramallo changed the title Component with translations example Component with translations Oct 18, 2019
@juanmanuelramallo
Copy link
Contributor Author

Hey @joelhawksley, I would appreciate your feedback on these changes! Thanks

@joelhawksley
Copy link
Member

@juanmanuelramallo thanks for the PR! I'm hoping to have time to look this over today.

@@ -0,0 +1,6 @@
# frozen_string_literal: true

class WithTranslations < ActionView::Component::Base
Copy link
Member

Choose a reason for hiding this comment

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

Might you be able to rename this to TranslationsComponent, to fit convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✔️

- Renamed `WithTranslations` to `TranslationsComponent`
Copy link
Member

@joelhawksley joelhawksley left a comment

Choose a reason for hiding this comment

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

👏 this looks good. I'll likely due a light refactor to extract looking up the component's filename from the initialize method in a followup PR ❤️

@joelhawksley joelhawksley merged commit f525f3e into ViewComponent:master Oct 18, 2019
self.class.instance_method(:initialize)
.source_location
.first
.gsub(%r{(.*app/)|(.rb)}, "")
Copy link

@eneagoe eneagoe Oct 24, 2019

Choose a reason for hiding this comment

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

Is it possible that the regex substitution is not entirely correct?

app/components/editorb_component.rb will be changed to components/edit_component instead of the expected "components/editorb_component".

The regex should probably be (%r{(.*app/)|(\.rb)} or (%r{(.*app/)|(.rb$)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems you're right 😞

[1] pry(main)> 'app/components/editorb_component.rb'.gsub(%r{(.*app/)|(.rb)}, '')
=> "components/edit_component"

Choose a reason for hiding this comment

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

we discussed (me and eugen) and likely the combination of eugen's proposal is the safest: (%r{(.*app/)|(\.rb$)} (meaning a litteral .rb at the end)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create a PR for this fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#87 created

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.

4 participants