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

Refactor Url chooser without XHR call #3432

Open
wants to merge 4 commits into
base: 7.x
Choose a base branch
from

Conversation

mart-insiders
Copy link

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets comma separated list of tickets fixed by the PR

@mart-insiders mart-insiders changed the base branch from 6.4 to 7.x August 29, 2024 09:00
@mart-insiders mart-insiders force-pushed the url-chooser branch 2 times, most recently from c1bba76 to 1e616c4 Compare September 30, 2024 05:54
Copy link
Member

@acrobat acrobat left a comment

Choose a reason for hiding this comment

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

Feature works well 👌 I've add a few comments and question but overall it looks alright!

Also can you rebase on the 7.2 branch and retarget the PR? I've already created the 7.2 release branch for the upcoming release!

$loader->load('services.yml');
$loader->load('commands.yml');

Copy link
Member

Choose a reason for hiding this comment

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

Can you add check to deprecate not settings this option?

        if (!$enableImprovedUrlchooser) {
            trigger_deprecation('kunstmaan/node-bundle', '7.2', 'Not setting the "kunstmaan_node.enable_improved_urlchooser" config to true is deprecated, it will always be true in 8.0.');
        }

Copy link
Author

Choose a reason for hiding this comment

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

added!

@@ -56,6 +56,8 @@ services:

kunstmaan_node.form.type.urlchooser:
class: Kunstmaan\NodeBundle\Form\Type\URLChooserType
arguments:
$improvedUrlChooser: '%kunstmaan_node.enable_improved_urlchooser%'
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use named parameters here as function parameter names are not covered by the BC policy

Copy link
Author

Choose a reason for hiding this comment

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

changed to unnamed parameter

if (field.name.indexOf($urlChooserName) !== -1 && field.name.indexOf('link_url') === -1) {
values[field.name] = field.value;
}
// deprecated, use adaptUrl(e.target) when enable_improved_urlchooser becomes standard
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what is exactly deprecated? So I understand what to remove in 8.0

Copy link
Author

Choose a reason for hiding this comment

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

The onclick event should trigger the function adaptUrl for the improved url chooser.
Everything below that is deprecated and only used for the current url chooser.
So, everything from r.222 var $form = $(this).closest('form'), untill r.264 }); is deprecated

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.

2 participants