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

Does not show anything in IE browser #5

Closed
hmoraes opened this issue Apr 7, 2016 · 12 comments
Closed

Does not show anything in IE browser #5

hmoraes opened this issue Apr 7, 2016 · 12 comments
Labels

Comments

@hmoraes
Copy link
Contributor

hmoraes commented Apr 7, 2016

I tested in Firefox, Chrome, IE11 and IE-Edge. In Firefox and Chrome it works well.

In IE browsers I found some errors like 'Symbol' is not defined. I fixed this error changing the foreach in the _loadBackdrop function to a array iteration. This fix also works in Firefox and Chrome. But in IE it does not show anything. I am trying to debug here and the difference between browsers is that in IE the

scrub-thumbnails has no children, no image-cache, no backdrop, no carousel, nothing.

You have any idea how to solve this problem?

@tjenkinson tjenkinson added the bug label Apr 7, 2016
@tjenkinson
Copy link
Owner

Thanks I think the last few commits should have fixed the symbol error. I'm not sure why nothing is getting added in IE and will have to spend some time looking into it.

@tjenkinson
Copy link
Owner

The .append()'s in render() don't seem to be working in IE for some reason

@hmoraes
Copy link
Contributor Author

hmoraes commented Apr 8, 2016

Exactly, this function works in other browsers but IE11.
In console the append() works. But in the plugin context not.

@hmoraes
Copy link
Contributor Author

hmoraes commented Apr 8, 2016

I found the problem. But I don't have a solution, yet.

When the MEDIACONTROL_RENDERED event is fired _onMediaControlRendered() function adds the element in MediaControl. So a clean copy is added in IE11.

I changed the event to call this.render instead. Now the element is filled. However when I play, the image-cache and carousel tags become empty and the carousel does not show the images, only turns black, but the spotlight show the image. This happens in all browsers.

Why you must add the element when the Media Control is rendered? This event is called at least 3 times before play, and one time after play? Any thoughts?

@tjenkinson
Copy link
Owner

Ah that's interesting. It is like this because if you look at the code in MediaControl you'll see before the MEDIACONTROL_RENDERED event is fired the media control is supposedly cleared with the this.$el.html(this.template({ settings: this.settings })) line.

If the element isn't removed, and the append is called again then I can see it might get a bit confused, but I thought the element would be being getting removed.

@tjenkinson
Copy link
Owner

I think when this.$el.html(this.template({ settings: this.settings })) happens it's not just removing my DOM element form the media control, but it's also clearing everything from inside it, which is the issue. On everything but IE it does what I expected and remove my element from the DOM, but not touch the children.

The behavior I expected which happens on everything but IE, is for this.$el.html(this.template({ settings: this.settings })) to remove my element from the DOM, and then my code just adds it back after the MEDIACONTROL_RENDERED event.

@hmoraes
Copy link
Contributor Author

hmoraes commented Apr 8, 2016

Got it.

Well, I did work but it is not a good solution. I put the initialization code in the event _onMediaControlRendered(). Works on all browsers but as this event is called multiple times.
I'll keep looking and try to find a better way

@tjenkinson
Copy link
Owner

Yes I was thinking a way of solving this would be to refactor it a bit and recreate and populate all the dom elements in _onMediaControlRendered(). I agree it isn't great for performance though with the render happening multiple times. The media control should not render very frequently though so it's not too bad.

I'm not sure why the media control renders multiple times on load, this may be a clappr bug which could be fixed. It might be worth listening to the player READY event as well and only enable the plugin after this, as this might fire after after the render ones.

@hmoraes
Copy link
Contributor Author

hmoraes commented Apr 9, 2016

It seems a good idea. I'll try here.

@hmoraes
Copy link
Contributor Author

hmoraes commented Apr 10, 2016

Hi

I tryed other events like CORE_READY and PLAYER_READY but didn't work.

I modified some things in your code and now it is working in all browsers (tested in Firefox, Chrome, IE-Edge and IE11). Also I removed the dependence of JQuery that reduced the script size from ~96KB to ~8Kb.

I did the following changes:

  • Remove JQuery dependency. This greatly reduces the final size of the script
    • Only the function outerWidth() was used from JQuery, but the diference between width() and outerWidth() in the spotlight was very small. Then outerWidth() was changed to width()
  • Changed the form of the creation of the plugin to use template
    • The tags for spotligth, backdropand backdropCarousel are added by the template
  • Fix the problem 'Symbol' not defined in IE 11
  • Rewrite the initialization to work in IE11
    • Loads the thumbnails using Image() object instead of adding in the tag _$imageCache
    • Search for _$spotligth, _$backdrop and _$backdropCarousel before use rather than maintaining the reference
    • Always call _render() and _init() on MEDIACONTROL_RENDERED event

I can do a Pull Request if you want.

Thanks for your work. It will be very useful for me

@tjenkinson
Copy link
Owner

@hmoraes thanks!

Yes please do send a PR. One of the things on my todo list was to remove jQuery so that's great.

@hmoraes
Copy link
Contributor Author

hmoraes commented Apr 10, 2016

I did the PR, see here #8

tjenkinson pushed a commit that referenced this issue Apr 10, 2016
* Fix build problem:

ERROR in ./~/css-loader!./~/sass-loader!./src/style.sass
Module build failed:
.scrub-thumbnails {
                  ^
      Invalid CSS after "...ub-thumbnails {": expected "}", was "{"
      in /home/heberte/terabit/clappr-thumbnails-plugin/src/style.sass (line 3, column 20)
 @ ./src/style.sass 4:14-116

Renaming src/style.sass to src/style.scss and change referencies in src/index.js and webpack.config.js

* Fix 'Symbol' not defined in IE browser

* - Remove JQuery dependency. This greatly reduces the final size of the script
   * Only the function outerWidth() was used from JQuery, but the diference between width() and outerWidth() in the spotlight was very small. Then outerWidth() was changed to width()
- Changed the form of the creation of the plugin to use template
   * The tags for spotligth, backdrop and backdropCarousel are added by the template
- Fix the problem 'Symbol' not defined in IE 11
- Rewrite the initialization to work in IE11
   * Loads the thumbnails using Image() object instead of adding in the tag _$imageCache
   * Search for _$spotligth, _$backdrop and _$backdropCarousel before use rather than maintaining the reference
   * Always call _render() and _init() on MEDIACONTROL_RENDERED event
- Build the release version with ~8KB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants