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

BREAKING(get rid of inner component) #81

Merged
merged 9 commits into from Nov 3, 2017
Merged

BREAKING(get rid of inner component) #81

merged 9 commits into from Nov 3, 2017

Conversation

@kybishop
Copy link
Owner

kybishop commented Nov 1, 2017

Initial spike of removing the inner component and switching to ember-popper 0.7.0, which uses a new approach for interacting with the popper element.

  • Get dummy app working
    • Make sure options can still be toggled (aka fix bug in ember-popper)
    • Fix weird style bug
    • Fix renderInPlace (should add tests to prevent regressions) kybishop/ember-popper#58
  • Get tests working
  • Make sure animations work as before
  • Fix issue where position is altered between arg changes (like arrow true/false)
@kybishop kybishop force-pushed the ember-popper-0.7.0 branch from 0d46e5a to 46ae0a4 Nov 1, 2017
kybishop added 5 commits Nov 2, 2017
@kybishop kybishop requested a review from rwwagner90 Nov 3, 2017
@kybishop

This comment has been minimized.

Copy link
Owner Author

kybishop commented Nov 3, 2017

I'll squash these commits when it's finally time to merge.

No rush, but @rwwagner90 I'd love your thoughts on this before I push it through.

kybishop added 2 commits Nov 3, 2017
@kybishop kybishop changed the title WIP: BREAKING(get rid of inner component) BREAKING(get rid of inner component) Nov 3, 2017
@rwwagner90

This comment has been minimized.

Copy link
Collaborator

rwwagner90 commented Nov 3, 2017

@kybishop can you give me a TLDR of why we made this switch? There is no inner component at all now? Just one component? I'm curious how it effects the issues I was having with outer and inner classes and showing and hiding.

@kybishop

This comment has been minimized.

Copy link
Owner Author

kybishop commented Nov 3, 2017

Hey @rwwagner90, here are the bullet points:

  1. The inner component was able to be removed thanks to ergonomic improvements to ember-popper.
  2. class is now passed directly to the inner div
  3. {{#ember-attacher}} is now {{#attach-popover}}
  4. The popper <div> (the outer div) is now hidden/shown, and receives the aria-hidden attribute.

We can't remove the inner <div> entirely because popper.js's positioning CSS conflicts with the animation CSS. To get around the issue, we apply animations to the inner div instead.

In regards to your previous issues with class, they'd disappear entirely with this PR. This effectively gets us to parity/compatibility with ember-tooltips.

@rwwagner90

This comment has been minimized.

Copy link
Collaborator

rwwagner90 commented Nov 3, 2017

@kybishop sounds good to me! I briefly looked at the code, and didn't see anything glaringly wrong, so fine with me to ship it 👍

@kybishop kybishop merged commit 733dc03 into master Nov 3, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@kybishop kybishop deleted the ember-popper-0.7.0 branch Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.