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

Add multiple container support (optional). #337

Merged
merged 3 commits into from Apr 16, 2019

Conversation

Projects
None yet
3 participants
@mudafar
Copy link
Contributor

mudafar commented Apr 10, 2019

Hi @fkhadra,

We faced an issue similar to these mentioned here:
#134

I tried to fix the issue with no luck, so I ended up with this PR, in theory it pass all the test, and the contribution recommendation, nevertheless let me know what do you think and let me know if I need to add/modify anything :)

Basically I included an explanation about the logic and how to use it in the readme.

The idea is to ensure there is no duplication of any toast inside and outside of a deployed component which use react-toastify internally. (parent and child using the library).

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 10, 2019

Coverage Status

Coverage increased (+0.08%) to 95.069% when pulling 319a0c5 on mudafar:master into d985bec on fkhadra:master.

@fkhadra

This comment has been minimized.

Copy link
Owner

fkhadra commented Apr 10, 2019

Hey @mudafar,

This look good 👌but may I ask you to write the corresponding test. If you don't know how I can point you to the right direction.

Thanks a lot for the PR BTW !

@fkhadra
Copy link
Owner

fkhadra left a comment

Waiting for Tests.

@mudafar

This comment has been minimized.

Copy link
Contributor Author

mudafar commented Apr 10, 2019

Hey @fkhadra,

Sure I'll add the corresponding test and let you know!

I'm glad that you liked the PR.

@mudafar

This comment has been minimized.

Copy link
Contributor Author

mudafar commented Apr 10, 2019

Hi @fkhadra ,

Test added, please when you've time check them and let me know.

I faced something strange, and it turns out that any test that create a toast after this one:

  it("Should throw an error if can't render a toast", () => {
    expect(() => {
      mount(<ToastContainer />);
      toast(false);
      jest.runAllTimers();
    }).toThrow(/The element you provided cannot be rendered/);
  });

Will keep throwing the same boolean type error, I couldn't find a solution for that!

Please let me know what you think about the tests.

Thanks!

@fkhadra

This comment has been minimized.

Copy link
Owner

fkhadra commented Apr 11, 2019

Hey @mudafar,

I'll check ASAP don't worry.

@mudafar

This comment has been minimized.

Copy link
Contributor Author

mudafar commented Apr 15, 2019

Hi @fkhadra ,

Please let me know if the tests are ok, and/or I need to add anything else.

Thanks!

@fkhadra fkhadra self-requested a review Apr 15, 2019

@fkhadra
Copy link
Owner

fkhadra left a comment

Hey @mudafar,

Your tests are ok. Great work BTW! The issue with the test below is due to the fact that there is no cleanup. false is still in the render queue.

  it("Should throw an error if can't render a toast", () => {
    expect(() => {
      mount(<ToastContainer />);
      toast(false);
      jest.runAllTimers();
    }).toThrow(/The element you provided cannot be rendered/);
  });

I'll add a comment to tell that this test need to be the last. Later on I'll add an api to clean this, so don't worry for the moment and excuse my laziness😆.

Got 2 remarks:

  • For the ToastContainer the prop is called containerId. But to display a toast the prop is called toastContainerId. Naming both props containerId feels more natural to me. What do you think ?
<ToastContainer containerId={1} />
toast('hello', { containerId: 1 });
  • Regarding the prop enableMultiContainer, I'm wondering if we can remove it and rely on the containerId only. So if the containerId is defined we opt-in for the multi container mode.
// just check that the containerId is set and valid
    if (this.hasContainerId()) {
      if (!this.belongToContainer(options)) {
        return null;
      }
    }
// lighter api but less explicit
<ToastContainer containerId={1} /> 

I never intended to use the library with multi container, so I'm not sure about that one. As a end user what feels more natural to you ?

Sorry for the long review by the way. And thank you again for the great work and effort 👌!

EDIT: codeclimate issue fixed

@fkhadra fkhadra added the feature label Apr 15, 2019

@mudafar

This comment has been minimized.

Copy link
Contributor Author

mudafar commented Apr 15, 2019

Hey @fkhadra ,

Thanks for your detailed review, and don't worry about the test thing :)

About the remarks:

  1. You're right, it's better to unify to containerId in both cases.
  2. I thought about it too, IMHO the idea behind enableMultiContainer is to:
    2.1. Make it explicit (as you already mentioned).
    2.2. Prevent activating the multiple container unintentionally.
    2.3. Prevent a complex logic for the end user, e.g: if you add containerId, this also will activate multiple container etc..., as per the Single Responsibility Principle
    2.4. Keep it optional as possible (it's not a common use-case).

I'll change the containerId already changed and pushed, let me know what do you think about the second remark.

Great review 👍

@fkhadra

This comment has been minimized.

Copy link
Owner

fkhadra commented Apr 16, 2019

@mudafar I think we are ready to release it now 😁. Thank you

@mudafar

This comment has been minimized.

Copy link
Contributor Author

mudafar commented Apr 16, 2019

Hi @fkhadra ,

Great news, please let me know when it's released, I need to use this feature.

Thank you in advance!

@fkhadra fkhadra merged commit 319a0c5 into fkhadra:master Apr 16, 2019

2 checks passed

codeclimate Approved by Fadi Khadra.
Details
coverage/coveralls Coverage increased (+0.08%) to 95.069%
Details
@fkhadra

This comment has been minimized.

Copy link
Owner

fkhadra commented Apr 16, 2019

hey @mudafar, it's published to npm 😎

@mudafar

This comment has been minimized.

Copy link
Contributor Author

mudafar commented Apr 17, 2019

Great, thank you @fkhadra !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.