-
Notifications
You must be signed in to change notification settings - Fork 0
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
Overlapping slider documentation #1
Conversation
Merge main to docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for such informative documentation. We still have some to solve though.
README.md
Outdated
```html | ||
<!-- No Js init --> | ||
<div data-overlapping-slider> | ||
<!-- Slider 1 --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It should be "Slide 1" instead of "Slider 1".
- Just put in 3 slides for demo, keep it as simple, as succinct as possible, like:
<!-- Init with HTML attribute -->
<div data-overlapping-slider>
<div>Slide 1</div>
<div>Slide 2</div>
<div>Slide 3</div>
<div>
By looking at this demo HTML, we would immediately know what we need to have to run the script, no extra things in the demo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you read the number 2 one more time? @vuquangpham
|-----------------------------|--------------------------------------|--------------------------------------------| | ||
| `data-os-autoplay="2` | `autoplay: 2` | Add autoplay option through html attribute | | ||
| `data-os-swipe` | `swipe: true` | Add swipe option through html attribute | | ||
| `data-overlapping-slider` | `data-overlapping-slider="slider-1"` | Pass an specific id for each slider | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute should be placed on top as it is the main one for initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved it to the top (after the initialize section). Please check it again!
❌ Deploy Preview for overlapping-slider failed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check my comments. Thanks.
README.md
Outdated
|-----------------------------|----------------------------------------------------------------------------------------| | ||
| `onPause: (data) => {}` | Trigger after turning off the autoplay setting. | | ||
| `onPlay: (data) => {}` | Trigger after turning on the autoplay setting. | | ||
| `onChange: (data,el) => {}` | When you call this event, your Overlapping Slider will re-initialize with new options. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trigger after turning off the autoplay setting.
- When talking about an event, the verb "fire" would be more accurate than "trigger".
- I can't find anything in common between
onChange
andre-initialize with new options
.
|-----------------------------|--------------------------------------|--------------------------------------------| | ||
| `data-os-autoplay="2` | `autoplay: 2` | Add autoplay option through html attribute | | ||
| `data-os-swipe` | `swipe: true` | Add swipe option through html attribute | | ||
| `data-overlapping-slider` | `data-overlapping-slider="slider-1"` | Pass an specific id for each slider | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget this one?
README.md
Outdated
```html | ||
<!-- No Js init --> | ||
<div data-overlapping-slider> | ||
<!-- Slider 1 --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you read the number 2 one more time? @vuquangpham
Add documentation for Overlapping Slider