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

Allow spotlight styles to be updated from outside #954

Closed
wants to merge 3 commits into from

Conversation

manishPh
Copy link

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.

When <Joyride /> 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 <Joyride /> with react-pdf to create highlighted section in the pdf. So we need finer control over the spotlight CSS being displayed in our app.
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 12, 2023

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 bdbc629:

Sandbox Source
React Configuration

@manishPh
Copy link
Author

@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!

src/components/Overlay.tsx Outdated Show resolved Hide resolved
@gilbarbara
Copy link
Owner

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?

@manishPh
Copy link
Author

@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.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@manishPh
Copy link
Author

@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.

@manishPh
Copy link
Author

manishPh commented Dec 1, 2023

@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.

@gilbarbara
Copy link
Owner

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.

@gilbarbara gilbarbara closed this Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants