essetwide / material-walkthrough

A material tour (eg Inbox from Google).
https://essetwide.github.io/material-walkthrough/
Apache License 2.0
32 stars 10 forks source link

Walk is broken when an element is too large #46

Closed NathanGrimaud closed 6 years ago

NathanGrimaud commented 6 years ago

Hello :) I had an issue while testing my project on a mobile phone. It looks like when an element of a walk is too big, the walk is broken. Here is a showcase : here https://codepen.io/grimu/project/editor/ZKVJpg . You can see in this pen that the demo is stuck, but if you zoom out, you will be able see the next steps.

menosprezzi commented 6 years ago

Hello Nathan! Nice that you have using our plugin. Note that the concept behind the walkthrough is based on the Feature Discovery proposal from Google. It's nice to use it to focus on elements that are "blurry" to the user view on the first view of your page/app. If the element that you need to focus is big enough to contain a portion of the size of the screen, it doesn't have to be focused.

NathanGrimaud commented 6 years ago

Ok I think I should not focus on those big elements then :) But I think that when this happens (if the users have zoomed a bit too much, or it's screen is too small) the library should throw an error (with a callback), or at least close the walker, as the app is unusable.

To be more generic, when the lib finds that itCanBeRenderedIn(Right|Top|...) is false for every directions, it should throw an error, or close the walker :)

If you are intrested in such a feature, i can take some time to create a PR :)

menosprezzi commented 6 years ago

It's a good point @NathanGrimaud, it's an awesome idea! You can do it! Thanks a lot! We'll track your work.

NathanGrimaud commented 6 years ago

Do you have any preferences to manage those error case :

we could add an error function to every WalkPoint, or we could add the error function to an option object passed to the walk function WalkPoint:

MaterialWalkthrough.walk([{
    target:"",
    content:"",
    color:"",
    acceptText:"",
    onSet:()=>{},
    onClose: ()=>{},
    onError: (error) => {}
}])

global options :

MaterialWalkthrough.walk([{
    target:"",
    content:"",
    color:"",
    acceptText:"",
    onSet:()=>{},
    onClose: ()=>{},
}], {
    onError: (error) => {}
})

Btw both can be added :)

menosprezzi commented 6 years ago

@NathanGrimaud, I think it would be better to make it possible to treat errors only in global options. And in the arguments of the onError function would be passed the information of which WalkPoint the walker broke. And if the developer dont pass the onError callback, it will be nice if the library launches a console.warn to warn the user that the walker has closed by a usage error.

NathanGrimaud commented 6 years ago

I just edited #47. I removed walk specific errors, and added the walkpoint as parameter to 'global' errors :) Now default behavior is to console.warn the user instead of using the built-in_log('ERROR', msg)

oliveirafilipe commented 6 years ago

Hi @NathanGrimaud , sorry for th delay. We're a little busy in the day work but we will take a look on this.

You mentioned that:

Now default behavior is to console.warn the user instead of using the built-in_log('ERROR', msg)

I think that maybe the _log function should know when to use console.log/error/warn or the plugin could use the Browser Support of the debug package. What do you think @menosprezzi ?

NathanGrimaud commented 6 years ago

Hi @oliveirafilipe @menosprezzi, have you found the way you want me to log those errors to the console ?

oliveirafilipe commented 6 years ago

Yes, I talked yesterday with @menosprezzi about it. I will commit some updates in your pull, really really soon. Maybe within 1 week

Em qui, 23 de ago de 2018 11:31, NathanGrimaud notifications@github.com escreveu:

Hi @oliveirafilipe https://github.com/oliveirafilipe @menosprezzi https://github.com/menosprezzi, have you found the way you want me to log those errors to the console ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/essetwide/material-walkthrough/issues/46#issuecomment-415438659, or mute the thread https://github.com/notifications/unsubscribe-auth/AVAzeisx2KDMbrEsKl30EM0qXMA8l2URks5uTrzGgaJpZM4VS5m1 .

NathanGrimaud commented 6 years ago

Great news !

oliveirafilipe commented 6 years ago

Closing this issue because i created a more specific one #48