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

feature: add support for multiple elements highlight #2069

Closed
wants to merge 0 commits into from

Conversation

IArny
Copy link

@IArny IArny commented Sep 16, 2022

I would propose feature to support multiple elements highlight. All elements selected with attachTo.element will be highlighted if attachTo.multiple is true. I added multiple flag to attachTo field to keep old behavior. I'd appreciate your feedback for this PR.

@IArny
Copy link
Author

IArny commented Sep 16, 2022

Also this PR can resolve #525 issue

@IArny
Copy link
Author

IArny commented Sep 19, 2022

@rwwagner90 could you review this PR please

@IArny
Copy link
Author

IArny commented Sep 22, 2022

@monshan Could you find a time for this PR?

index.html Outdated
@@ -271,7 +271,7 @@ <h3 class="demo-heading font-heading text-2xl uppercase">
</div>

<div class="hero-followup font-heading mb-12 mt-12 lg:mb-24 lg:mt-24">
<div class="bg-navy inline-block mb-4 w-56 lg:mr-4">
<div class=" bg-navy inline-block mb-4 w-56 lg:mr-4">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<div class=" bg-navy inline-block mb-4 w-56 lg:mr-4">
<div class="bg-navy inline-block mb-4 w-56 lg:mr-4">

@@ -125,6 +125,31 @@
}
],
id: 'attaching'
},
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{
{

text: 'Your tour steps can highlight multiple elements (like this step).',
attachTo: {
element: '.hero-multiple-select',
on: 'bottom',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of weird spacing issues in here. Could you please auto format this with prettier?

};
if (targetElements) {
const elements = [];
targetElements.forEach(el => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing issues here as well

@RobbieTheWagner
Copy link
Member

Thanks for the PR @IArny! Lots of formatting issues with the spacing in this code, please run prettier and clean things up and I will take another look. I think in general we would love to support this feature, but I am not sure if this is the best API for it.

@IArny IArny force-pushed the master branch 2 times, most recently from 15bf7d4 to 9a8cbc3 Compare September 29, 2022 11:33
@IArny IArny closed this Sep 29, 2022
@IArny
Copy link
Author

IArny commented Sep 30, 2022

@rwwagner90 thank you for your response. I closed this PR by mistake, but already opened the new one #2083 . I ran prettier and it fixed a little bit more files than were changed by me, hope it's fine.

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