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

Add custom apps translation scripts and image path for consistency #40898

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

akhil1508
Copy link
Contributor

Summary

  • Add checks for custom app translations and images

Checklist

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

I'm unsure about this change in general.
I have plenty of apps in apps-extra and see the requests for the translation files.

lib/private/Template/JSResourceLocator.php Outdated Show resolved Hide resolved
@akhil1508
Copy link
Contributor Author

akhil1508 commented Oct 13, 2023

I'm unsure about this change in general.
I have plenty of apps in apps-extra and see the requests for the translation files.

For me, occ app:install calendar installed it in custom_apps folder. I don't see the requests for custom_apps/calendar/l10n/$lang.js for any language. Nor if I modify via theme..

Ah I recall now it is set through the apps_paths config

@kesselb kesselb added bug 3. to review Waiting for reviews labels Oct 17, 2023
@kesselb kesselb added this to the Nextcloud 28 milestone Oct 17, 2023
@kesselb
Copy link
Contributor

kesselb commented Oct 17, 2023

@juliushaertl @skjnldsv I'm totally unsure if the patch makes sense, we need it, etc .. 🙈

However, I dislike the ability to modify files from apps via a theme.

@akhil1508 akhil1508 force-pushed the dev/custom-apps-assets branch from 94a02b6 to e30c3de Compare October 17, 2023 15:39
@juliusknorr
Copy link
Member

I'm all in for dropping the theme generally once we have proper ways to achieve its mainly used functionality otherwise #27433

I think for now the change makes sense, to ensure that the legacy theme mechanism works as expected with custom app paths.

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Would be good to squash the commits into one, otherwise this seems good.

@akhil1508 akhil1508 force-pushed the dev/custom-apps-assets branch from e30c3de to 5e20c3e Compare October 19, 2023 09:10
@akhil1508
Copy link
Contributor Author

Would be good to squash the commits into one

@juliushaertl Squashed

@akhil1508 akhil1508 requested a review from kesselb October 25, 2023 11:54
@kesselb kesselb requested review from a team, ArtificialOwl, icewind1991, susnux and artonge and removed request for a team, kesselb, ArtificialOwl and icewind1991 October 26, 2023 19:27
@juliusknorr
Copy link
Member

Failures seem unrelated.

@juliusknorr juliusknorr merged commit 51eb44d into nextcloud:master Oct 30, 2023
34 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom apps translation and image asset handling is not consistent with core apps
4 participants