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

Translatable #39

Closed
wants to merge 21 commits into from
Closed

Conversation

scottgrayson
Copy link

@scottgrayson scottgrayson commented Sep 28, 2022

  • require translatable package and updated requirements for php and db
  • make attributes translatable for survey models
  • add indexes to db tables

@matt-daneshvar
Copy link
Owner

Hi @scottgrayson and @andreia, thank you for using laravel-survey and for taking the time to create this PR.

I think both spatie/eloquent-sortable and spatie/laravel-translatable are great choices you made in your work, but I'm cautious about adding them as new dependencies to this package, mostly because:

  • This will lock us to those packages.
    • E.g. we will need to start tracking spatie/laravel-translatable and ensure to tag new versions of laravel-survey as and when new major versions of spatie/laravel-translatable are released.
    • E.g. if Spatie chooses to stop supporting a specific version of PHP/Laravel in their future releases, we will have to do the same. This may not always be in the best interest of our audience.
  • We can create Dependency Hell for our users.
    • E.g. a user may already depend on spatie/laravel-translatable v5.x in their app. They will end up in a difficult situation if laravel-survey requires spatie/laravel-translatable v6.x.

These make me think that it'd be better to leave it to users to install spatie/laravel-translatable directly in their app and extend laravel-survey models and override them as and when they need to.

Leaving the PR open for discussion.

@delete-merged-branch delete-merged-branch bot deleted the translatable branch October 26, 2022 12:23
@swilla swilla restored the translatable branch October 26, 2022 12:24
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.

None yet

3 participants