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

feature: Remove support for setting nonstandard attributes as props #7857

Merged
merged 6 commits into from
Jul 28, 2022

Conversation

roman-bc-dev
Copy link
Contributor

@roman-bc-dev roman-bc-dev commented Jul 27, 2022

Amend the createEl method on the Dom class to remove logic that allows role, type, and aria-* attributes to be set as part of the properties object on an element.

Changes include:

  • Remove conditional statement from createEl method that checks for nonstandard attributes in the properties object and sets them as element attributes.
  • Remove duplicate tabIndex attribute assignment in Slider.creatEl method.

Specific Changes proposed

Please list the specific changes involved in this pull request.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #7857 (363d9dd) into next (4cdb2ab) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             next    #7857      +/-   ##
==========================================
+ Coverage   81.60%   81.78%   +0.17%     
==========================================
  Files         110      111       +1     
  Lines        7350     7373      +23     
  Branches     1777     1779       +2     
==========================================
+ Hits         5998     6030      +32     
+ Misses       1352     1343       -9     
Impacted Files Coverage Δ
src/js/slider/slider.js 59.29% <ø> (ø)
src/js/player.js 89.21% <100.00%> (+0.39%) ⬆️
src/js/title-bar.js 100.00% <100.00%> (ø)
src/js/utils/dom.js 64.28% <100.00%> (+0.34%) ⬆️
src/js/video.js 94.11% <0.00%> (-0.05%) ⬇️
src/js/tech/tech.js 83.42% <0.00%> (ø)
src/js/utils/url.js 78.04% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Looks good with one small change!

textContent(el, val);
} else if (el[propName] !== val || propName === 'tabIndex') {
el[propName] = val;
}
});

Object.getOwnPropertyNames(attributes).forEach(function(attrName) {
el.setAttribute(attrName, attributes[attrName]);
Object.entries(attributes).forEach(([attrName, val]) => {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately we can't use entries. Our minimum Chromium version will be 53 (because of WebOS 4.x) and it wasn't added until Chromium 54. 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, quick change there.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

:shipit:

@roman-bc-dev roman-bc-dev merged commit 6a8e156 into videojs:next Jul 28, 2022
@roman-bc-dev roman-bc-dev deleted the update-createel branch July 28, 2022 20:45
misteroneill pushed a commit that referenced this pull request Nov 23, 2022
…7857)

* remove statement that handles attributes in props argument

* clean up function

* add unit test

* add unit test

* remove duplicate set attr

* revert function cleanup
misteroneill pushed a commit that referenced this pull request Nov 23, 2022
…7857)

* remove statement that handles attributes in props argument

* clean up function

* add unit test

* add unit test

* remove duplicate set attr

* revert function cleanup
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
…ideojs#7857)

* remove statement that handles attributes in props argument

* clean up function

* add unit test

* add unit test

* remove duplicate set attr

* revert function cleanup
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.

2 participants