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

Fixes #432 - Create association on locale change #433

Closed
wants to merge 1 commit into from

Conversation

xprazak2
Copy link
Contributor

According to the docs, calling MyPage.instance.header
should get the header text for current locale.
However, the associations are set up in a singleton
for a default locale and never modified.

The language-specific methods like MyPage.instance.header_en
still work, but it would be nice to have fae handle the locale
for us.

This change checks for languages in fae_fields for page
and dynamically changes the association if the locale change
is detected.

@xprazak2 xprazak2 force-pushed the dynamic-rebind branch 6 times, most recently from a8ee2b4 to 06d45b1 Compare October 14, 2018 15:04
@xprazak2
Copy link
Contributor Author

There is no way I can reasonably satisfy code climate - if I use the guard clause, then I get flagged for the line length.

@johnnyshields
Copy link
Contributor

johnnyshields commented Nov 9, 2018

I would suggest to remove the singleton altogether. Does it really add a substantial performance benefit? The code would be cleaner and more intuitive then.

@xprazak2
Copy link
Contributor Author

I removed the singleton as suggested.

@xprazak2 xprazak2 force-pushed the dynamic-rebind branch 3 times, most recently from 661966b to 8b785da Compare February 9, 2019 15:04
According to the docs, calling MyPage.instance.header
should get the header text for current locale.
However, the associations are set up in a singleton
for a default locale and never modified.

The language-specific methods like MyPage.instance.header_en
still work, but it would be nice to have fae handle the locale
for us.

This change checks for languages in fae_fields for page
and dynamically changes the association if the locale change
is detected.
@xprazak2
Copy link
Contributor Author

xprazak2 commented Feb 9, 2019

Fixing tests, can't get around the line length/guard clause rules combo.

@jamesmk
Copy link
Member

jamesmk commented Apr 24, 2019

@xprazak2 I'm sorry if the docs weren't clear, but auto-translating attrs is provided by the fae_translate method:

def fae_translate(*attributes)

This method is in a concern for user generated models and doesn't currently support Fae::StaticPage.

The @singleton_is_setup is crucial for performance reasons, otherwise all the methods would have to be redefined every time you request an instance. So the meta programming can't change based on I18n.locale.

However, it would be possible to omit the base attr name here and then layer on a dynamic method similar to fae_translate. So Fae::StaticPage would define header_en and header_cs and layed on top would be a dynamic method like:

translated_attributes.each do |attribute|
  define_method attribute.to_s do
    self.send "#{attribute}_#{I18n.locale}"
  end
end

I'm going to close out this and open a ticket. I do like the refactoring you made in the model, so if you want to include that or and attempt at the above, I'll make it a priority.

thanks!

@xprazak2
Copy link
Contributor Author

Adding fae_translate :field_name to static page as described in #472 works flawlessly, thanks!

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.

3 participants