Skip to content
This repository has been archived by the owner on May 22, 2021. It is now read-only.

Chain jQuery calls, do not use events alias and store selectors #444

Merged
merged 1 commit into from
Aug 5, 2017
Merged

Chain jQuery calls, do not use events alias and store selectors #444

merged 1 commit into from
Aug 5, 2017

Conversation

Johann-S
Copy link
Contributor

@Johann-S Johann-S commented Aug 4, 2017

This PR add some improvements about perfs and good practices.

  • Remove events shortcuts ( .click transformed to .on('click')
  • Chain jQuery calls
  • Store selectors to avoid DOM access when we had already find the DOM elements
  • Change $(document).ready to $(function () { }) with jQuery 3 it's recommended to use this form to listen the DOM ready events (see : https://api.jquery.com/ready/)

Fix : #284

$copyBtn.html(
'<img src="/resources/check-16.svg" class="icon-check"></img>'
);
window.setTimeout(() => {
$copyBtn.attr('disabled', false);
$('#link').attr('disabled', false);
$link.attr('disabled', false);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could potentially move $link.attr() up a line and chain both $copyBtn.attr() calls.
I think you can also set multiple values in a single .attr() by passing an object: http://api.jquery.com/attr/#attr-attributes

$link.attr('disabled', false);
$copyBtn.attr({
  disabled: false,
  'data-l10n-id': 'copyUrlFormButton'
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed that thank you @pdehaan 👍

@dannycoates dannycoates merged commit 2a7099a into mozilla:master Aug 5, 2017
@dannycoates
Copy link
Contributor

Thanks @Johann-S 👍

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants