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

Separate modal overlay opening element option #1837

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wbobeirne
Copy link

Closes #1836

  • Adds modalOverlayOpeningElement option for specifying an HTMLElement or string selector for what the modal should target
    • Added documentation for this too
    • Added a cypress test for this new option, but in order to test this, I also added a getOpeningProperties 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.
  • Refactored a little in general to reuse a parseElementOrSelector function for the new option and attachTo.element
    • Added a unit test for new parseElementOrSelector function

@wbobeirne
Copy link
Author

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.

@RobbieTheWagner
Copy link
Member

@wbobeirne you can see the failing tests by clicking details next to them.

Seems like the test for undefined attachTo is broken https://github.com/shipshapecode/shepherd/runs/5489452652?check_suite_focus=true

@stale
Copy link

stale bot commented Apr 16, 2022

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.

@RobbieTheWagner
Copy link
Member

@wbobeirne would you be able to rebase this with master? We've made some changes, so perhaps tests will work again.

@monshan
Copy link
Contributor

monshan commented Jun 28, 2022

@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

@RobbieTheWagner
Copy link
Member

@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
@monshan monshan force-pushed the modal-overlay-opening-element branch from 824acae to 3b1f4d5 Compare June 28, 2022 23:51
@RobbieTheWagner
Copy link
Member

@monshan looks like we have some failing tests here still. How are things looking?

@monshan
Copy link
Contributor

monshan commented Jul 11, 2022

@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 attachTo takes a function.

@RobbieTheWagner
Copy link
Member

@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 attachTo takes a function.

Ah yes, that sounds correct.

@wbobeirne could you please refactor this to be compliant with the new attachTo.element supporting functions?

@RobbieTheWagner
Copy link
Member

@EmNicholson93 I think this needs some refactoring to be compliant with the new attachTo.element API. Can you please take a look?

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

Successfully merging this pull request may close these issues.

Separate selectors for modal overlay cutout target and popper target
3 participants