buildo / react-cookie-banner

React Cookie banner which can be automatically dismissed with a scroll. Because fuck The Cookie Law, that's why.
http://react-components.buildo.io/#cookiebanner
MIT License
182 stars 19 forks source link

dismissOnClick feature #28

Closed codeheroics closed 7 years ago

codeheroics commented 7 years ago

This is a proposal for a dismissOnClick feature which would allow a click anywhere in the whole banner to dismiss it.

My use case: I'm can't use dismissOnScroll, but correctly clicking on an small icon can actually be kinda hard on mobile, so I'd like the banner to be more easily dismissable,.

gabro commented 7 years ago

Thanks for you PR, I think another viable approach (and possibly more flexible), would be to extend the mechanism introduced in #25, i.e. passing a function as child which receives onClick as parameter. Extending the implementation of #25, it would look like:

<CookieBanner>
  {(onAccept, onClick) => <MyOtherComponent onClick={onClick} />}
 </CookieBanner>

Now MyOtherComponent can invoke onClick whenever it desires, covering also (but not only) your use case.

What do you think @codeheroics?

/cc @FrancescoCioria

FrancescoCioria commented 7 years ago

Hi @codeheroics

What you're asking for is IMO an opinionated solution to a precise problem.

Why don't you simply make the icon/button bigger when using the component from a mobile device? You can easily edit the CSS of the button and every other component of react-cookie-banner.

To solve your precise problem (dismiss after click on any part of banner) you could do something like this:

.react-cookie-banner {
  position: relative;

  .button-close {
    position: absolute;
    top: 0;
    left: 0;
    height: 100%;
    width: 100%;
  }
}
FrancescoCioria commented 7 years ago

@gabro what you're proposing is actually already implemented: onAccept is the function used to dismiss the banner (you can see it here).

So, to add a possible solution to @codeheroics issue, we could solve it with a custom banner template like this:

<CookieBanner>
  {(onAccept) => <MyCustomBanner onDismiss={onAccept} />}
</CookieBanner>
codeheroics commented 7 years ago

Hi @gabro, @FrancescoCioria

While I do understand that dismissOnClick is quite opinionated, isn't dismissOnScroll just the same? I do like it but unfortunately, I can't get people at work to allow it, so I'm trying to find another good solution :)

I actually think that the ideal solution would be something like a dismissOn props which would take events as values, something like <Banner dismissOn={['scroll']} /> by default, which I could customize to <Banner dismissOn={['click', 'scroll']} />.

The CSS solution you suggest @FrancescoCioria bothers me because it breaks the placement of the close icon, and I still need it to be present at its current position.

However, if I could, like you suggest, get the children React component to get the onAccept props, it would already solve a good portion of my problem (although it removes some of the interest of React-Banner-Cookie for me, which I thought was a good drop-in solution, as I now have to deal with a child component)

gabro commented 7 years ago

I'll leave @FrancescoCioria the answer to your other concerns, as he's the owner of this project.

I'll just say about this

However, if I could, like you suggest, get the children React component to get the onAccept props

that it's already implemented (since the latest release).

As of today you can already do:

<CookieBanner>
  {(onAccept) => <div onClick={onAccept}>whatever</div>}
</CookieBanner>
codeheroics commented 7 years ago

Woops, thought I was up to date but I'm actually still on 0.0.16, sorry, going to update right away

codeheroics commented 7 years ago

So I correctly get the onAccept props, sorry about that! However that approach forces me to:

At this point, in order to get the ability to dismiss the banner on click, I removed most of what react-banner-cookie offered me (a simple solution to get the banner & ability to dismiss it), the only reason left for me to use it being its ability to set & check the cookie to display or not the banner, which I could use any cookie library for.

FrancescoCioria commented 7 years ago

I see you point and I agree with you on saying that it would reduce the benefits you get from this library.

The real problem here is that the two main features of this component (automatic cookie management and standard template) collide in your scenario. I think the right way to solve your issue would be to separate the cookie management logic and the banner template in two different components: CookieBanner (default) and Content.

The Content component would be exported as well and used as default by Cookie banner and it would accept any template related prop.

If we do this your problem could be solved as follows:

<CookieBanner {...cookieProps}>
  {(onAccept) => <div onClick={onAccept}><Content {...templateProps} /></div>
</CookieBanner>

What do you think @codeheroics, would this be a good solution for you?

codeheroics commented 7 years ago

Sorry for the late answer, I'm currently on holidays.

I think that it's an acceptable solution, but I really don't like the need to add the arrow function to get onAccept.

Would you be against injecting the onAccept props to the children React node you get? eg.

<CookieBanner {...cookieProps}>
  <Content {...templateProps} />
</CookieBanner>

and in CookieBanner's rendering method, you'd clone the children props to inject onAccept to it?

FrancescoCioria commented 7 years ago

I see a problem: after React 0.14 there's no real distinction between components and functions which means I can't know if a function children a is a plain function or a react component (in the first case it would be an error to use cloneWithProps).

Also, here at buildo we tend to avoid implicit APIs as we prefer easy-to-read over easy-to-write :)

As we agree the main problem is the interdependence of template and logic, I'll close this PR and open a dedicated issue to start a PR to separate logic and template asap.

Thank you very much @codeheroics for pointing out this problem!