Skip to content
This repository has been archived by the owner on Aug 30, 2019. It is now read-only.

Improve side panel implementation to get rid of data-rel #244

Open
spzm opened this issue Nov 23, 2017 · 2 comments
Open

Improve side panel implementation to get rid of data-rel #244

spzm opened this issue Nov 23, 2017 · 2 comments

Comments

@spzm
Copy link
Contributor

spzm commented Nov 23, 2017

Description

this.closeButtons = [].slice.call(this.panel.querySelectorAll('[data-rel="close"]'));
this.closeButtons.forEach(b => b.addEventListener('click', this.handleClose));

For now SlidePanel goes though all elements that have data-rel="close" in the children (through real dom) and add native click event listener for them. I see it as convenient solution to have close button in any place of SlidePanel. I propose to update the implementation just to more explicit way.

Steps to reproduce

We found several issues:

  • When children content will be re-rendered - subscription can be lost.
  • Next code is used as an example but shows possible issue in the future. In react child ComponentDidMount calls first - then parent one. It means that child component close button will have two handlers.
<SidePanel>
  <button data-rel="close" />
  <SidePanel>
    <button data-rel="close" />
  </SidePanel>
</SidePanel>

Proposal

I propose to try component as a function approach here to pass close handlers to the children. Please comment if you have better or alternative solutions!

<SlidePanel>
  {(close) => {
    return (
       ... some content
       <button onClick={close} />
       ... some content
    );
  }}
</SlidePanel>
@asci
Copy link
Contributor

asci commented Nov 23, 2017

it could be implemented backward compatible

@mAiNiNfEcTiOn
Copy link
Contributor

Interesting idea! If you don't have time for it I can pick it up, if you don't mind 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants