Fix popover flyout issue #1637
Fix popover flyout issue #1637
Conversation
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 23b6796 (Please note that this is a fully automated comment.) |
Codecov Report
@@ Coverage Diff @@
## master #1637 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 395 395
Lines 8110 8110
Branches 1196 1196
=======================================
Hits 8109 8109
Misses 1 1
Continue to review full report at Codecov.
|
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: b5f0db0 (Please note that this is a fully automated comment.) |
Autocomplete has no trigger button
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 1bd9343 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 7391c46 (Please note that this is a fully automated comment.) |
Blackbaud-StacyCarlos
changed the title
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 680e59f (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 25df94a (Please note that this is a fully automated comment.) |
| @@ -44,7 +44,7 @@ const FLYOUT_CLOSED_STATE = 'flyoutClosed'; | |||
| styleUrls: ['./flyout.component.scss'], | |||
| animations: [ | |||
| trigger('flyoutState', [ | |||
| state(FLYOUT_OPEN_STATE, style({ transform: 'translateX(0)' })), | |||
| state(FLYOUT_OPEN_STATE, style({ transform: 'inherit' })), | |||
There was a problem hiding this comment.
@Blackbaud-StacyCarlos Thanks for the pull request. While I think your other changes could be helpful, as it stands, I only added this change to master branch and the dropdowns worked. Can you explain the research that went into this change?
There was a problem hiding this comment.
The main aspect that I used to fix this came from the fact that popovers are all fixed elements. Fixed elements don't, or rather should not, have an offsetParent. Despite that, the popover was behaving as if it was absolute positioned inside the flyout.
With that knowledge I knew that some styling of the flyout must have been causing this. I immediately realized it was the transform. The transform styling creates a stacking context which pretty much means that everything within it is treated like one big unit so that it will move together based on your specifications. Honestly, it should probably be set to initial instead of inherit, but I was just thinking of some random cases at the time.
Does that help?
There was a problem hiding this comment.
@Blackbaud-StacyCarlos Thanks for the in-depth reply; certainly helps (I can confirm that initial also works).
Now that we know this minimal change fixes the respective issue, I'm leaning toward pulling this single change out into a new pull request and keeping the other, more involved changes in this branch for future consideration.
It would also be nice to add a simple dropdown to the Flyout's visual test, since this behavior is primarily visual.
Thoughts on that? If we do it that way, we can probably merge and release this fix much quicker. (I'm also happy to do this for you, if you do not have spare time.)
There was a problem hiding this comment.
I can do this immediately after I get back from lunch.
There was a problem hiding this comment.
Perfect. Thank you!
There was a problem hiding this comment.
@Blackbaud-SteveBrush Sorry for the delay. I have separated the pull requests and added an appropriate visual test.
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 8037df2 (Please note that this is a fully automated comment.) |
…aud#1535 (blackbaud#1652)" This reverts commit 349a5d4.
'initial' instead of 'inherit' translate as well.
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: f3e495d (Please note that this is a fully automated comment.) |
Blackbaud-StacyCarlos
changed the title
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 97652d3 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: b411887 (Please note that this is a fully automated comment.) |
This incorporates a fix for #1636.
The use ofgetBoundingClientRectis fine, but the resultingtop,bottom,right, andleftare not based on theoffsetParentof the original element.When specifyingleftandtopof a positioned element, we must ensure we compensate for the position of the first positioned ancestor, i.e.offsetParent, if you want to to line up with other siblings. Originally, the was solved by appending to body, but the change in design warrants this fix.Rather than adding logic to determine offset parent, I have opted to remove styling that forces offset parents for fixed elements.
However, I do believe many of these components are fundamentally flawed in how they were implemented. The reuse of the entire dropdown component, rather than a simple popover or dropdown menu component, was good as a base, but should be refactored out to increase maintainability and readability.
The plunkr for Flyouts is currently broken, but this is where the issue can be seen.