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

Allows for local step asynchronous events #1266

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

Conversation

ezra-obiwale
Copy link

  • Individual steps can now provide onbeforechange(next) and onchange() event callbacks
  • When provided, the local events would be called while the global events would not.

Bonus:

  • Adds class no-title to the tooltip header layer if a step doesn't have a title
  • Adds step class (step-[STEP_NUMBER]) to the reference layer

- Removes duplicate loops
- Implements step events
- Adds "no-title" class when a step doesn't have a title
- Adds the current step class ("step-[number]")
- Adds tests
- Adds css classes "waiting" and the direction to the reference layer
when waiting
- Disables clicking next/previous while waiting
if (targetElement.title) {
oldTooltipHeaderLayer.classList.remove('no-title');
} else {
oldTooltipHeaderLayer.classList.add('no-title');
Copy link
Contributor

Choose a reason for hiding this comment

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

How this no-title class will be used? I couldn't find any CSS changes in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it's for themes to use if they need it.

For instance, in my use case, I needed to hide the header element if there's no title.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, got it. I personally think we should keep the codebase simple and avoid adding any classes that is not used right now. I understand that you need that no-title class in your application though. Can you use your initial input (e.g. your steps list) to see if a specific step as title or not?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, thought of that but that's JS. It's better to use CSS if possible instead of JS.

About avoiding adding classes, it's not unusual for libraries to add css classes that may help developers customize the outlook, even if the library doesn't use the classes.

this._options.scrollTo,
disableInteraction,
};
if (step > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a scary function to change mainly because we don't have any tests for it.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, the point is to use a single loop, instead of two. As such, existing tests already cover this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we have tests for this? there are two ways of adding the steps, using the data-* attributes and a JSON object. I don't think we have any tests for the data-* attr approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just started refactoring this function and adding some tests. I will ping you once it's merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

WIP #1295

@DamienCassou
Copy link

I would be interested in this feature.

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.

3 participants