Allow arrow padding to be configured for a step. #3051
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@JakeThurman is attempting to deploy a commit to the shipshapecode Team on Vercel. A member of the Team first needs to authorize it. |
|
@RobbieTheWagner Any chance this could make it in soon? I'd rather not create an special fork for my company to use, and it sounds like this is important to have for us. |
|
Hi @JakeThurman apologies for the extremely late reply on this! Is this something that could be fixed via CSS in your app? We do heavy arrow customization via CSS for our landing page with things like this: .shepherd-element[data-popper-placement^=top]>.shepherd-arrow {
bottom:-8px
} |
|
@RobbieTheWagner No worries on the delay. I appreciate you taking the time to reply now! It's certainly possible, but it would only work in isolated cases. floating-ui has lots of logic to make sure the arrow points at the anchor element, and this would eliminate that down to just a fixed number of px from the edge. (i.e., middleware can shift the This floating-ui padding parameter configures the closest the arrow can get to the edge of the balloon. It seems to be set to 4px because of the default shepherd CSS. Finally, the floating-ui documentation explicitly says to set this parameter to your border-radius, which is what led me to open this PR so I could do so. See: https://floating-ui.com/docs/arrow#padding |
shepherd.js/src/utils/floating-ui.ts
Outdated
| options.middleware.push( | ||
| arrow({ element: arrowEl, padding: hasEdgeAlignment ? 4 : 0 }) | ||
| arrow({ element: arrowEl, padding: hasEdgeAlignment ? arrowOptions.padding : 0 }) |
There was a problem hiding this comment.
| arrow({ element: arrowEl, padding: hasEdgeAlignment ? arrowOptions.padding : 0 }) | |
| arrow({ element: arrowEl, padding: hasEdgeAlignment ? step.options.arrow?.padding ?? 4 : 0 }) |
What would you think about putting this inline here? I think it is a little less verbose.
There was a problem hiding this comment.
This doesn't work since the existing model allows arrow to be a boolean. (Which I kept to avoid a breaking change). We need to handle the case where step.options.arrow is a boolean which is why I wrote this way I did.
|
Pushed a fixup for the requested changes. I wasn't able to run my updated tests... I know I did originally when opening this PR, but I couldn't remember how to run them now. |
|
@JakeThurman apologies for the delay on this! The commands you will want to run tests are from the root: |
JakeThurman
force-pushed
the
modify-arrow-padding
branch
from
2329da4 to
2ad296f
Compare
Thanks! Updated my fixup to get the tests working. |
|
Thanks @JakeThurman I think things are looking good now! One final request, could you please update the docs to mention this? The auto generated docs will be updated based on your comments, but we have some hand rolled docs too like https://github.com/shipshapecode/shepherd/blob/main/docs-src/src/content/docs/guides/usage.md#-displaying-arrows |
Done. I did a grep and didn't find any other documentation that appeared to need updating. |
| const arrowOptions = | ||
| typeof step.options.arrow === 'object' | ||
| ? step.options.arrow | ||
| : { padding: 4 }; | ||
|
|
There was a problem hiding this comment.
| const arrowOptions = | |
| typeof step.options.arrow === 'object' | |
| ? step.options.arrow | |
| : { padding: 4 }; |
| arrow({ element: arrowEl, padding: hasEdgeAlignment ? 4 : 0 }) | ||
| arrow({ | ||
| element: arrowEl, | ||
| padding: hasEdgeAlignment ? arrowOptions.padding : 0 |
There was a problem hiding this comment.
| padding: hasEdgeAlignment ? arrowOptions.padding : 0 | |
| padding: hasEdgeAlignment ? step.options.arrow?.padding ?? 4 : 0 |
There was a problem hiding this comment.
Hi! I had responded to this above. This doesn't work since step.options.arrow can still be a boolean.
There was a problem hiding this comment.
I think this would be working javascript (I.e., (true).padding is undefined) but typescript doesn't allow that.
There was a problem hiding this comment.
Oh, gotcha. Didn't realize TS wouldn't allow it.
|
Thanks @JakeThurman! I think my suggestions might have gotten lost from before. I left a couple more, please let me know what you think of them. |
Hi! First time contributer.
We have a >4px border radius on out shepherd-element, and our looking to increase it so the arrow doesn't overlay the border radius:
This PR allows you to configure the arrow padding using something like:
arrow: { padding: 10 }This builds and tests pass locally, please let me know if there's anything else you need from me on this PR, or anything I missed!