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

[POC] Replace x-editor #8221

Open
wants to merge 1 commit into
base: 5.x
Choose a base branch
from

Conversation

onEXHovia
Copy link
Contributor

Subject

Example of possible implementation for replacement xeditor to classic way. On the first request we create a form with an editable field and display in popover, after successfully submit form, we update the field in the list.

This is a very quick implementation to discuss, I am open to ideas and suggestions. This approach gives more flexibility than xeditable because everything is built around symfony forms.

Changelog

### Added

### Changed

### Deprecated

### Removed

### Fixed

### Security

@VincentLanglet
Copy link
Member

Can u add screenshot before/after ?
Thanks

@onEXHovia
Copy link
Contributor Author

Can u add screenshot before/after ? Thanks

The appearance is the same as xeditable
edit

@VincentLanglet VincentLanglet requested review from a team and jordisala1991 December 7, 2024 10:08
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

I like the idea, this would help to bump to boostrap 4 (or 5) #7156

  • @sonata-project/contributors WDYT ?
  • We need to decide if it requires to be releases as 5.0 or if it can be considered as BC.
  • CI need to be fixed

@onEXHovia
Copy link
Contributor Author

I like the idea, this would help to bump to boostrap 4 (or 5) #7156

My plans include updating the frontend(#7156) and replacing adminlte to bootstrap 5.*, I think all the changes should be in the release of the 5.x branch.

@@ -213,6 +213,7 @@ const Admin = {

setup_xeditable(subject) {
Admin.log('[core|setup_xeditable] configure xeditable on', subject);
return;
Copy link
Member

Choose a reason for hiding this comment

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

This is intended? Either leave a comment with a TODO or remove the code below

@core23
Copy link
Member

core23 commented Dec 7, 2024

I like the idea, this would help to bump to boostrap 4 (or 5) #7156

  • @sonata-project/contributors WDYT ?
  • We need to decide if it requires to be releases as 5.0 or if it can be considered as BC.
  • CI need to be fixed

We could do it in the 5.0, but this will take months or even years to be released...

Looking at EasyAdmin, they allow minor breaking changes on stable branches, with all the downsides.
At the moment we also have a CVE in our stable branch that could not be fixed without dropping support for bootstrap 3 / 4, which will also be a big breaking change.

@zyberspace
Copy link
Contributor

zyberspace commented Dec 7, 2024

Hi @onEXHovia ,

thanks for the work! This is indeed an important step to get the Admin LTE update working (one of many 😄 ).

Regarding BC:

I just looked at our more heavily modified sonata admin projects and I would prefer to have a feature flag in the sonata_admin config, e.g. enable_new_editable, that throws a deprecation if set to the default false value. API-Platform did this with more than one feature for the 3.x branch and the update was a breeze thanks to it. This way i was able to fix one deprecation at a time and do multiple updates until everything was fixed.

Deprecation example: https://github.com/api-platform/core/blob/3.4/src/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php#L1032-L1034

->booleanNode('keep_legacy_inflector')->defaultTrue()->info('Keep doctrine/inflector instead of symfony/string to generate plurals for routes.')->end()

// ...

 if ($config['keep_legacy_inflector']) {
    trigger_deprecation('api-platform/core', '3.2', 'Using doctrine/inflector is deprecated since API Platform 3.2 and will be removed in API Platform 4. Use symfony/string instead. Run "composer require symfony/string" and set "keep_legacy_inflector" to false in config.');
}

Regarding the PR itself:

Would it be possible to add a few events to the stimulus controller to be able to modify the request options and maybe even the response? At least the options i could do via twig with the x-editable implementation.

Thanks!

@VincentLanglet
Copy link
Member

I think such PR can be done in 4.x

  • If there is no BC break in PHP.
  • If we get the same behavior in js.

I'm not sure we need to provide a enable_new_editable flag

@@ -38,5 +38,6 @@ module.exports = {
2,
],
'import/no-webpack-loader-syntax': 'off',
'lines-between-class-members': { 'properties': 'off', 'methods': 'always' }
Copy link
Member

Choose a reason for hiding this comment

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

this file should be edited on dev kit

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.

5 participants