Skip to content

Component with translations#74

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

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

Conversation

@juanmanuelramallo

@juanmanuelramallo juanmanuelramallo commented Oct 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

This was referenced Oct 17, 2019
Closed
Closed
@juanmanuelramallo
Component with translations
e38efca 
- Proposing a solution for the shortcut issue #49
@juanmanuelramallo juanmanuelramallo changed the title Component with translations example Component with translations Oct 18, 2019
@juanmanuelramallo

juanmanuelramallo commented Oct 18, 2019

Copy link
Copy Markdown
Contributor Author

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

@joelhawksley

joelhawksley commented Oct 18, 2019

Copy link
Copy Markdown
Member

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

joelhawksley
joelhawksley reviewed Oct 18, 2019
View reviewed changes
Comment thread test/app/components/with_translations.rb Outdated
@@ -0,0 +1,6 @@
# frozen_string_literal: true

class WithTranslations < ActionView::Component::Base

@joelhawksley joelhawksley Oct 18, 2019

Copy link
Copy Markdown
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?

@juanmanuelramallo juanmanuelramallo Oct 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done ✔️

@juanmanuelramallo
After code review
bc77e38 
- Renamed `WithTranslations` to `TranslationsComponent`
joelhawksley
joelhawksley approved these changes Oct 18, 2019
View reviewed changes

@joelhawksley joelhawksley left a comment

Copy link
Copy Markdown
Member

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
eneagoe
eneagoe reviewed Oct 24, 2019
View reviewed changes
Comment thread lib/action_view/component/base.rb
self.class.instance_method(:initialize)
.source_location
.first
.gsub(%r{(.*app/)|(.rb)}, "")

@eneagoe eneagoe Oct 24, 2019 •
edited
Loading

Copy link
Copy Markdown

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$)}

@juanmanuelramallo juanmanuelramallo Oct 24, 2019

Copy link
Copy Markdown
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"

@koenhandekyn koenhandekyn Oct 24, 2019

Copy link
Copy Markdown

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)

@juanmanuelramallo juanmanuelramallo Oct 26, 2019

Copy link
Copy Markdown
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

@juanmanuelramallo juanmanuelramallo Oct 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#87 created

@juanmanuelramallo juanmanuelramallo mentioned this pull request Oct 26, 2019
Merged
@elia elia mentioned this pull request Dec 11, 2020
Closed
@jcoyne jcoyne mentioned this pull request Nov 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelhawksley joelhawksley joelhawksley approved these changes

+2 more reviewers

@koenhandekyn koenhandekyn koenhandekyn left review comments

@eneagoe eneagoe eneagoe left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@juanmanuelramallo @joelhawksley @koenhandekyn @eneagoe