Closed manishPh closed 6 months ago
This pull request is automatically built and testable in CodeSandbox.
To see build info of the built libraries, click here or the icon next to each commit SHA.
Latest deployment of this branch, based on commit bdbc629579407f445eb3cd748b676de15e397cde:
Sandbox | Source |
---|---|
React | Configuration |
@gilbarbara can you please take a look at this very small change which could be really useful for lot of users of this awesome library. Thanks a bunch!
Hey @manishPh
I'm testing the changes locally.
I changed the spotlight
and spotlightLegacy
types to accept only numbers for the positioning attributes and extracted the parse to its function in the helpers' module.
But changing the spotlight position doesn't affect the tooltip position, so how will you be able to handle that in the PDF case?
@gilbarbara since my usecase is somewhat unique, I also rely on Step.offset
prop to manipulate the offset the way I want. Because in addition to left
, I also added in my change the ability to set the width
of the spotlight. So that math is helping me. I mean, this is just giving me more flexibility, most people don't have to use this, and it should continue to work as is.
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell
No Coverage information
0.0% Duplication
@gilbarbara hey, checking in if you were able to review/test this. Let me know if you have any comments on how to move forward on this if possible.
@gilbarbara hey, checking in if you were able to review/test this. Let me know if you have any comments on how to move forward on this if possible.
Hi @gilbarbara let me know if you need more time to test this out, or you think this is not something you want to merge/consider at this point.
Hey @manishPh I'm sorry for not getting back to you sooner.
I've decided against this change. As you said, it's a very niche case, and the implementation isn't great. This library is based on target elements, and only changing the spotlight position will result in a terrible UX. Since I don't know how you're implementing Joyride on top of a PDF (but you probably have HTML on the page), you could create transparent elements with absolute position and target them instead.
When is used as third party lib in host react applications, there are several instances where you might need a more finer control on how things are displayed. This change allows developers to manipulate spotlight style if they want according to their needs.
In our case, we are using with react-pdf to create highlighted section in the pdf. So we need finer control over the spotlight CSS being displayed in our app.