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

Popup: Add classes option #8068

Closed
wants to merge 0 commits into
base: 1.5-dev
from

Conversation

Projects
None yet
5 participants
@gabrielschulhof
Contributor

gabrielschulhof commented Apr 13, 2015

Fixes gh-7686

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 28, 2015

Member

@gabrielschulhof this is blocked by #8141 which is assigned to you and once that lands this will need modification based on it. Thats why I assigned it to you.

Member

arschmitz commented May 28, 2015

@gabrielschulhof this is blocked by #8141 which is assigned to you and once that lands this will need modification based on it. Thats why I assigned it to you.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 11, 2015

Member

@gabrielschulhof i blew this up when rebasing 1.5-dev with master can you fix it?

Member

arschmitz commented Jun 11, 2015

@gabrielschulhof i blew this up when rebasing 1.5-dev with master can you fix it?

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 15, 2015

Member

@gabrielschulhof is this ready for review i see you added commits?

Member

arschmitz commented Jun 15, 2015

@gabrielschulhof is this ready for review i see you added commits?

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jun 15, 2015

Contributor

Sorry! Yes, it's ready for review.

Contributor

gabrielschulhof commented Jun 15, 2015

Sorry! Yes, it's ready for review.

Show outdated Hide outdated js/widgets/popup.js
@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 16, 2015

Member

I don't see any of the tests for deprecated options moved into backcompat. Also everywhere you use classes assertions your accessing and passing the DOM node don't do this classes assertions accept jQuery objects.

Member

arschmitz commented Jun 16, 2015

I don't see any of the tests for deprecated options moved into backcompat. Also everywhere you use classes assertions your accessing and passing the DOM node don't do this classes assertions accept jQuery objects.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 16, 2015

Member

Also just noticed that you did not add anything to jscs

Member

arschmitz commented Jun 16, 2015

Also just noticed that you did not add anything to jscs

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jun 21, 2015

Contributor

I think I can remove all theme-related code from the arrow, because the arrow elements are inside the popup payload, not the popup container, and the popup payload is explicitly themed. Thus, the arrow can just always receive ui-body-inherit, and it does not need to react to on-the-fly theme changes at all. Using CSS to transmit the swatch from the payload to the arrow will greatly simplify the code.

Contributor

gabrielschulhof commented Jun 21, 2015

I think I can remove all theme-related code from the arrow, because the arrow elements are inside the popup payload, not the popup container, and the popup payload is explicitly themed. Thus, the arrow can just always receive ui-body-inherit, and it does not need to react to on-the-fly theme changes at all. Using CSS to transmit the swatch from the payload to the arrow will greatly simplify the code.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jul 3, 2015

Contributor

@arschmitz re backcompat tests: I should only be testing the arrow backcompat, because otherwise I'm simply using the widget backcompat module, which should have tests of its own, right?

Contributor

gabrielschulhof commented Jul 3, 2015

@arschmitz re backcompat tests: I should only be testing the arrow backcompat, because otherwise I'm simply using the widget backcompat module, which should have tests of its own, right?

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jul 14, 2015

Contributor

@arschmitz my link points to a file tests/integration/popup/backcompat-tests.html. It's a separate file.

Contributor

gabrielschulhof commented Jul 14, 2015

@arschmitz my link points to a file tests/integration/popup/backcompat-tests.html. It's a separate file.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jul 14, 2015

Member

weird when i followed it it pointed to regular core stuff and i even did a search for "backcompat" on the page and didn't find that file :-/ my bad!

Member

arschmitz commented Jul 14, 2015

weird when i followed it it pointed to regular core stuff and i even did a search for "backcompat" on the page and didn't find that file :-/ my bad!

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jul 14, 2015

Member

👍 just going to do device testing along with page

Member

arschmitz commented Jul 14, 2015

👍 just going to do device testing along with page

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jul 14, 2015

Contributor

Awesome!

Contributor

gabrielschulhof commented Jul 14, 2015

Awesome!

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jul 22, 2015

Member
  • demos/popup tooltip does not work on desktop but works ~50% on iphone
  • demos/popup light box popups broken
  • most of the pages in the different popup demos that have buttons with icons need to be updated
  • /demos/popup-outside-multipage does not work
  • demos/popup-iframe/ the first time the video player is opened it opens shows video and closes. Once the video is chached it does not happen any more so works fine even on refresh. WP 8.1 does not do this but scrolls you to the top of the page but again only first time.
  • windows phone 8.1 closing a popup brings you to top of page then back to where you were with a refresh its kinda odd. The dynamic popups do not do this.
  • "Note that large arrows may not be displayed at all. This is because their sides would "stick out" of the popup." this is not true as far back as the demo exists which is 1.4.0
  • The map demo does not load the map in firefox
Member

arschmitz commented Jul 22, 2015

  • demos/popup tooltip does not work on desktop but works ~50% on iphone
  • demos/popup light box popups broken
  • most of the pages in the different popup demos that have buttons with icons need to be updated
  • /demos/popup-outside-multipage does not work
  • demos/popup-iframe/ the first time the video player is opened it opens shows video and closes. Once the video is chached it does not happen any more so works fine even on refresh. WP 8.1 does not do this but scrolls you to the top of the page but again only first time.
  • windows phone 8.1 closing a popup brings you to top of page then back to where you were with a refresh its kinda odd. The dynamic popups do not do this.
  • "Note that large arrows may not be displayed at all. This is because their sides would "stick out" of the popup." this is not true as far back as the demo exists which is 1.4.0
  • The map demo does not load the map in firefox
@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jul 22, 2015

Contributor

demos/popup tooltip does not work on desktop but works ~50% on iphone

That's because the icon was not inside the anchor, but next to it. On the iphone you might get lucky and hit the adjacent anchor, but on the desktop, the mouse pointer almost certainly hits the icon and not the adjacent invisible anchor.

Contributor

gabrielschulhof commented Jul 22, 2015

demos/popup tooltip does not work on desktop but works ~50% on iphone

That's because the icon was not inside the anchor, but next to it. On the iphone you might get lucky and hit the adjacent anchor, but on the desktop, the mouse pointer almost certainly hits the icon and not the adjacent invisible anchor.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jul 22, 2015

Member

ah yup thats it exactly

Member

arschmitz commented Jul 22, 2015

ah yup thats it exactly

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jul 22, 2015

Contributor

/demos/popup-outside-multipage does not work

That's because $.fn.enhanceWithin() doesn't return this, but this.children(). #8175 fixes this.

Contributor

gabrielschulhof commented Jul 22, 2015

/demos/popup-outside-multipage does not work

That's because $.fn.enhanceWithin() doesn't return this, but this.children(). #8175 fixes this.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jul 22, 2015

Contributor

"Note that large arrows may not be displayed at all. This is because their sides would "stick out" of the popup." this is not true as far back as the demo exists which is 1.4.0

That is true, just not in the preview, because the preview is CSS-only. In a real popup, the placement code eliminates from consideration arrow orientations that would cause the base of the triangle to stick out of the popup. I'll change the blurb to reflect that.

Contributor

gabrielschulhof commented Jul 22, 2015

"Note that large arrows may not be displayed at all. This is because their sides would "stick out" of the popup." this is not true as far back as the demo exists which is 1.4.0

That is true, just not in the preview, because the preview is CSS-only. In a real popup, the placement code eliminates from consideration arrow orientations that would cause the base of the triangle to stick out of the popup. I'll change the blurb to reflect that.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jul 22, 2015

Member

Ah ok thats fine then just some copy update will be good

Member

arschmitz commented Jul 22, 2015

Ah ok thats fine then just some copy update will be good

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jul 22, 2015

Member

@gabrielschulhof with the popup video example iv seen this a lot before im trying to remember the cause i think this might not be a regression. I think it has to do with navigation going on within the iframe

Member

arschmitz commented Jul 22, 2015

@gabrielschulhof with the popup video example iv seen this a lot before im trying to remember the cause i think this might not be a regression. I think it has to do with navigation going on within the iframe

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jul 23, 2015

Contributor

windows phone 8.1 closing a popup brings you to top of page then back to where you were with a refresh its kinda odd.

I think we need to feature-detect Windows Phone 8.1, because it suffers from the same bug as earlier versions, and $.mobile.browser.oldIE evaluates to false there.

Contributor

gabrielschulhof commented Jul 23, 2015

windows phone 8.1 closing a popup brings you to top of page then back to where you were with a refresh its kinda odd.

I think we need to feature-detect Windows Phone 8.1, because it suffers from the same bug as earlier versions, and $.mobile.browser.oldIE evaluates to false there.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jul 23, 2015

Member

@gabrielschulhof that should be easy I think we can just look for pointer events support

Member

arschmitz commented Jul 23, 2015

@gabrielschulhof that should be easy I think we can just look for pointer events support

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jul 25, 2015

Contributor

@arschmitz I know you said I could land this after the last batch of changes, but I've actually made some significant changes, so please have another look at this before I land it.

Contributor

gabrielschulhof commented Jul 25, 2015

@arschmitz I know you said I could land this after the last batch of changes, but I've actually made some significant changes, so please have another look at this before I land it.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jul 25, 2015

Contributor

The popups no longer close like they did before, but on iOS 6.1, when you open the video popup while there's still navigation going on it jumps to the top, and you need to scroll back down to it.

Contributor

gabrielschulhof commented Jul 25, 2015

The popups no longer close like they did before, but on iOS 6.1, when you open the video popup while there's still navigation going on it jumps to the top, and you need to scroll back down to it.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jul 26, 2015

Member

👍 this looks good and i dont see the closing any more. 👍

Member

arschmitz commented Jul 26, 2015

👍 this looks good and i dont see the closing any more. 👍

gabrielschulhof added a commit that referenced this pull request Jul 29, 2015

@XZZsr

This comment has been minimized.

Show comment
Hide comment
@XZZsr

XZZsr Jul 29, 2015

I don't know !who can teach me!

XZZsr commented Jul 29, 2015

I don't know !who can teach me!

@gabrielschulhof gabrielschulhof deleted the gabrielschulhof:7686-popup-classes-option branch Jul 29, 2015

arschmitz added a commit to arschmitz/jquery-mobile that referenced this pull request Jul 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment