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

Related models should have the main entry locale too #4359

Merged
merged 5 commits into from
Jun 29, 2022

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented May 5, 2022

WHY

BEFORE - What was wrong? What was happening before this PR?

If you have for example an HasMany relation where some fields of the related model are translatable, it will always display the default locale, ignoring the main entry locale.

  • So you are editing the main entry in french, it will save the related fields in the according language, but when displaying the main entry in french, the related fields are displayed in the default language (english).

AFTER - What is happening after this PR?

The related fields are edited in the same language as the main entry.

HOW

How did you achieve that, in technical terms?

setup the locale on model when fetching the related models from database.

Is it a breaking change?

I don't think so, it was not working at all.

How can we test the before & after?

Having translatable fields inside a relationship, there is nothing like that setup in demo, if you want I can create something there to test this.

@guleswine
Copy link
Contributor

guleswine commented Jun 10, 2022

Come on guys, I'm tired of adding this manually with every update😀

@guleswine
Copy link
Contributor

guleswine commented Jun 29, 2022

@pxpm just in case, I want to check if you forgot about it😀

@pxpm
Copy link
Contributor Author

pxpm commented Jun 29, 2022

@guleswine 🙃 🙃 Sorry sorry! I think what's preventing @tabacitu from merging this is that this should be merged with other PR that introduces the useFallbackLocale.

I will remove it from here, since it's not mandatory for this to work, to make this PR more appealing to @tabacitu merging skills 👍

Give me a sec I will push the change.

@pxpm
Copy link
Contributor Author

pxpm commented Jun 29, 2022

Done, ping @tabacitu anything preventing you from merging this now ?

Cheers

@tabacitu tabacitu merged commit b3205f6 into v5 Jun 29, 2022
@tabacitu tabacitu deleted the related-models-should-have-the-main-entry-locale-too branch June 29, 2022 09:14
@tabacitu
Copy link
Member

Ahhh... much cleaner! Thank you @pxpm , love it when it's so easy to understand what's wrong and what's the fix.

@guleswine we've merged it, it'll be available with a composer update starting Monday. Thank you for your patience 🙏 And the nudge 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minor Bug A bug that happens only in a very niche or specific use case. Priority: MUST
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants