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

Fixed Mobile support #25

Merged
merged 5 commits into from
Oct 6, 2016
Merged

Fixed Mobile support #25

merged 5 commits into from
Oct 6, 2016

Conversation

alxlark
Copy link
Contributor

@alxlark alxlark commented Oct 3, 2016

Fixed issue #6. Add stop counting on click

@ja1984
Copy link
Owner

ja1984 commented Oct 3, 2016

@alxlark since this should be a fix for mobile only, maybe we should use something like touchstart/touchend for the counter not to change behavior on desktop?

https://developer.mozilla.org/en-US/docs/Web/API/Touch_events

@alxlark
Copy link
Contributor Author

alxlark commented Oct 3, 2016

@ja1984 I think about this, but apply for PC too, cause for example if I want show message for friend or senior staff I can't do it cause message disappear in few seconds.

@ja1984
Copy link
Owner

ja1984 commented Oct 3, 2016

@alxlark the countdown should stop when you hover the notification?

@alxlark
Copy link
Contributor Author

alxlark commented Oct 3, 2016

@ja1984 correct it stops for hover notification, but how about if notifications few? we can only hover one of them, but clicking on needed itmes we can hold it to show in future.

messages

@ja1984
Copy link
Owner

ja1984 commented Oct 3, 2016

I see your point, but maybe click should toggle stop-counting at least?

A use case would be that you make an action with a retry button instead of dismiss, you click notification to show some one, then there is no way for that notification to get removed?

@alxlark
Copy link
Contributor Author

alxlark commented Oct 4, 2016

Click that actually do, it add class stop-counting and clear timer. If next time user trigger mouseleave this function check if stop-counting class present if no it runs start counting other way exit.

We can hide message by click on dismiss button, this functionality was untouched.

You can check how it work here https://cdn.rawgit.com/alxlark/jackbox/issue-6_mobile_support/index.html

@ja1984
Copy link
Owner

ja1984 commented Oct 4, 2016

Yes, but the problem is that once you have stopped an notification you can not start it again, and lets say instead of a dismiss button you have a retry or undo-button.

Then there is no way for the notification to get removed :)

@alxlark
Copy link
Contributor Author

alxlark commented Oct 4, 2016

If we don't have delete button, so the same problem will be present on mobile device and we also will can't remove message. We need decide what we do with flow and then code it.

Also if we refactor to touch event, need think about behavior on notebooks touch screens that support both mouse and touch.

@ja1984
Copy link
Owner

ja1984 commented Oct 4, 2016

The default behaviour is to have a dismiss button (icon or text) however, you should be able to add a callback function, it will replace the default button with another action button.

So you will not be able to manually close the notification, you will have to let it time out.

But if we interfere with the timeout there will be no way of removing notification.

I like the idea of being able to "pause" the timeout, I think however that this should be toggled.

@alxlark
Copy link
Contributor Author

alxlark commented Oct 4, 2016

So in this case we need on second click just clear class stop-counting and all will goes by old way. Implement clearing class on second click?

@ja1984
Copy link
Owner

ja1984 commented Oct 4, 2016

I think , rename stopCounter to toggleCounter in that method, check if notification has stop-counter class, if so remove it, else add it.

I'm sorry I can't show with code, I'm on my phone

@alxlark
Copy link
Contributor Author

alxlark commented Oct 4, 2016

Good, anyway I'll update code in the evening

}

var stopCounter = function () {
notification.classList.add("stop-counting");
Copy link
Owner

Choose a reason for hiding this comment

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

notification.classList.toggle("stop-counting")

@alxlark
Copy link
Contributor Author

alxlark commented Oct 4, 2016

@ja1984 I add toggle version, please review

@ja1984 ja1984 merged commit c38321d into ja1984:master Oct 6, 2016
@ja1984
Copy link
Owner

ja1984 commented Oct 6, 2016

👍

@ja1984 ja1984 mentioned this pull request Oct 6, 2016
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