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

Overlapping slider documentation #1

Merged
merged 11 commits into from
Oct 7, 2022
Merged

Overlapping slider documentation #1

merged 11 commits into from
Oct 7, 2022

Conversation

vuquangpham
Copy link
Contributor

Add documentation for Overlapping Slider

@phucbm phucbm self-requested a review October 1, 2022 03:35
@phucbm phucbm added the documentation Improvements or additions to documentation label Oct 1, 2022
Copy link
Member

@phucbm phucbm left a 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 -->
Copy link
Member

Choose a reason for hiding this comment

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

  1. It should be "Slide 1" instead of "Slider 1".
  2. 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.

Copy link
Member

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 |
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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!

@netlify
Copy link

netlify bot commented Oct 5, 2022

Deploy Preview for overlapping-slider failed.

Name Link
🔨 Latest commit 772677c
🔍 Latest deploy log https://app.netlify.com/sites/overlapping-slider/deploys/633faa210436e3000af465fb

Copy link
Member

@phucbm phucbm left a 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. |
Copy link
Member

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.

  1. When talking about an event, the verb "fire" would be more accurate than "trigger".
  2. I can't find anything in common between onChange and re-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 |
Copy link
Member

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 -->
Copy link
Member

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

@phucbm phucbm merged commit 23e5178 into main Oct 7, 2022
@phucbm phucbm deleted the docs branch October 7, 2022 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants