search

sandflow / imscJS

JavaScript library for rendering IMSC Text and Image Profile documents to HTML5
BSD 2-Clause "Simplified" License
84 stars 31 forks source link

Replace for..in loops with index-based for loops #210

Closed xoundboy closed 3 years ago

xoundboy commented 3 years ago

I had auto whitespace stripping enabled in Webstorm so there's a lot of whitespace removal too.. I hope this isn't going to be a problem. Thanks.

palemieux commented 3 years ago

I plan to run the regression tests and get back to you.

xoundboy commented 3 years ago

Perfect, thanks

palemieux commented 3 years ago

@xoundboy It looks like the changes might have gone a little too far. For example, styleAttrs and node.attributes and head.layout.regions are in fact dictionaries, so for..in is appropriate, right? In other words, only iteration over arrays should be changed to for(..;..;..).

xoundboy commented 3 years ago

@palemieux I reverted my previous commit and started again. I just fixed those problems that were actually preventing subtitles from being parsed on Tizen TVs and dropped all of the other overzealous changes. I also resorted to the hasOwnProperty checks for safety. Thanks

palemieux commented 3 years ago

@xoundboy Thanks. Apologies for the delay. I expanded your PR at https://github.com/sandflow/imscJS/pull/211 to include all for loops (and also corrected a bug). Any chance you might be able to try it on the tizen platform? The regression tests pass on my side.

palemieux commented 3 years ago

@xoundboy Quick ping.

palemieux commented 3 years ago

Replaced by #211