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

Improve jQuery code #7141

Merged

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented May 1, 2021

Avoid some deprecations and use arrow functions as much as possible.
Try to avoid using this, and instead use the parameters passed to the
function.

Subject

I am targeting this branch, because this is where it can be applied.

Part of #7049

I think it is pedantic since it does not fix any issue, the js deprecations are not showin anywhere. (and probably they are deprecated on 3.0 of jQuery not on 2.0)

Avoid some deprecations and use anonymous functions as much as possible.
Try to avoid using this, and instead use the parameters passed to the
function.
@@ -48,7 +48,7 @@ const Admin = {
}
},
read_config() {
const data = $('[data-sonata-admin]').data('sonata-admin');
const data = jQuery('[data-sonata-admin]').data('sonata-admin');
Copy link
Member Author

Choose a reason for hiding this comment

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

Some consistency here, try to use always jQuery so it can be easily identified

@@ -94,8 +94,8 @@ const Admin = {
if (Admin.get_config('USE_SELECT2')) {
Admin.log('[core|setup_select2] configure Select2 on', subject);

jQuery('select:not([data-sonata-select2="false"])', subject).each(function eachSetupSelect2() {
const select = jQuery(this);
jQuery('select:not([data-sonata-select2="false"])', subject).each((index, element) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

arrow functions preferred over normal functions

checkboxClass: 'icheckbox_square-blue',
radioClass: 'iradio_square-blue',
})
// See https://github.com/fronteed/iCheck/issues/244
Copy link
Member Author

Choose a reason for hiding this comment

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

This issue looks like it was solved some time ago

@@ -190,16 +185,16 @@ const Admin = {
const isChecked = jQuery(`tbody input[type="checkbox"]:nth(${currentIndex})`, subject).prop('checked');

// Check all checkbox between previous and current one clicked
jQuery('tbody input[type="checkbox"]', subject).each((i, e) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

don't like 1 char var name

@jordisala1991 jordisala1991 marked this pull request as ready for review May 1, 2021 14:38
@jordisala1991 jordisala1991 requested review from VincentLanglet and a team May 1, 2021 14:38
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.

Any config we should add to eslint to promote arrow function (or other things) ?

@jordisala1991
Copy link
Member Author

jordisala1991 commented May 1, 2021

I think the default airbnb config is just fine.

The thing with arrow function is that sometimes (when you use this) you can't use arrow functions. But since the each function (and most of the callbacks from jquery already provide a way to acces this via function parameter), we should really use that and be able to throw an arrow function. This way we don't end up with a lot of this around the code that we don't know what they are.

So, at the end we should encourage arrow functions, but there might be some cases where normal functions are needed. Airbnb eslint already takes care when you are trying to use a normal function without using this, and converts it to arrow function.

@jordisala1991 jordisala1991 merged commit 0f0cf90 into sonata-project:master May 2, 2021
@jordisala1991 jordisala1991 deleted the feature/add-entry-point branch May 2, 2021 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants