-
-
Notifications
You must be signed in to change notification settings - Fork 650
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Separate modal overlay opening element option #1837
base: main
Are you sure you want to change the base?
Separate modal overlay opening element option #1837
Conversation
Just tried the failing tests locally and they're passing for me. What's the best way to get more info out of CI? I couldn't find a way to get at any specific error logs or the screenshots. |
@wbobeirne you can see the failing tests by clicking details next to them. Seems like the test for |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@wbobeirne would you be able to rebase this with master? We've made some changes, so perhaps tests will work again. |
@rwwagner90 I looked into this but @wbobeirne 's fork is password protected so I can't push my rebase to his branch, lmk your thoughts if its worth cloning and opening a new PR |
@monshan if you use the GitHub desktop app, you can pull down this PR instead of a branch. That might be a good way to pull it down, rebase, and push it back up to this PR. |
…ement to draw overlay around Added re-usable `parseElementOrSelector` function for new arg and `attachTo.element` Rebases with master
Also added getOpeningProperties function to modal class to easily verify in testing
824acae
to
3b1f4d5
Compare
@monshan looks like we have some failing tests here still. How are things looking? |
@rwwagner90 I thought the scope of this one would just be to rebase it which went through but still not passing. If I remember right its because this change conflicts with the Shepherd 10 upgrade because now the |
Ah yes, that sounds correct. @wbobeirne could you please refactor this to be compliant with the new |
@EmNicholson93 I think this needs some refactoring to be compliant with the new |
Closes #1836
modalOverlayOpeningElement
option for specifying anHTMLElement
or string selector for what the modal should targetgetOpeningProperties
method to modal so I could confirm the dimensions matched up with the desired element. Let me know if you'd rather this test be written in a different way that didn't require changing the API of modal.general
to reuse aparseElementOrSelector
function for the new option andattachTo.element
parseElementOrSelector
function