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

Extract some javascript from main Admin and base script #7145

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented May 1, 2021

Subject

I am targeting this branch, because this can only be done on master.

Part of #7049

Changelog

### Changed
- Refactor internal Sonata Assets to split them in smaller components

If we like this idea, I can refactor in small PRs all the current Javascript (and I am sure I can do it for 4.0 release, in time)

@jordisala1991
Copy link
Member Author

WDYT about this kind of refactor @sonata-project/contributors . My idea is that it does not affect the Admin object that is accesible outside the scripts, so it won't break BC.

The thing is. IMO we should provide a way for people to build its own sonata js, to do so with some guarantees, we should provide some scripts that are kind of "public", but have some other that are internal.

Maybe we should provide:

app.js in case you want default scripts and want to add some more thing
admin.js in case you want default sonata scripts and add your own third party dependencies (like if you want to upgrade to admin-lte 3 in your own project)

And the rest we could treat them as internal OR expose them too. IMO since the files are not all well refactored, I am more of the opinion of internal, but at some point, with good refactored code, we might mark them as all public and live with it...

Example:

I want to build my own js and I pick all dependencies from my own node_modules because I want to upgrade admin-lte and I pick most of the sonata js code, BUT I want to override the getConfig function, because I want to pick config from somewhere else.
I do not know if that makes sense at all, but let's talk about it.

@jordisala1991 jordisala1991 force-pushed the feature/refactor-admin branch from 614ae74 to 58c2d63 Compare May 1, 2021 15:46
@SonataCI
Copy link
Collaborator

SonataCI commented May 2, 2021

Could you please rebase your PR and fix merge conflicts?

@jordisala1991 jordisala1991 force-pushed the feature/refactor-admin branch from 58c2d63 to dfb4980 Compare May 2, 2021 07:12
@jordisala1991 jordisala1991 changed the title Feature/refactor admin Extract some javascript from main Admin and base script May 2, 2021
@jordisala1991 jordisala1991 marked this pull request as ready for review May 2, 2021 17:39
@jordisala1991
Copy link
Member Author

Please review @sonata-project/contributors . I need your opinions to move this forward or not. This is needed if we want to give to create some documentation on how to customize the assets, and in order to integrate the inline scripts

@VincentLanglet VincentLanglet requested a review from a team May 2, 2021 23:29
@jordisala1991 jordisala1991 marked this pull request as draft May 4, 2021 07:24
@jordisala1991
Copy link
Member Author

I want to pass this as draft for now. Maybe there are other important things to do on Sonata before taking this one.

@SonataCI
Copy link
Collaborator

SonataCI commented May 5, 2021

Could you please rebase your PR and fix merge conflicts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants