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

[NV-2168] ✨ Polishing: Fix docs and types for the header #3285

Closed
2 tasks done
david-morris opened this issue Apr 25, 2023 · 1 comment
Closed
2 tasks done

[NV-2168] ✨ Polishing: Fix docs and types for the header #3285

david-morris opened this issue Apr 25, 2023 · 1 comment
Labels
linear polishing Created by Linear-GitHub Sync triage

Comments

@david-morris
Copy link
Contributor

david-morris commented Apr 25, 2023

πŸ“œ Description

I had to read the source to realize that not only was it possible to use an existing settings component from Novu, but I just needed to accept a setScreen callback in the header to use the existing transition in the popup component.

πŸ‘Ÿ Reproduction steps

N/A

πŸ‘ Expected behavior

  • header prop of the notification popup should accept a React.FC<{setScreen: (screen: ScreensEnum)=> void}> instead of a ()=>JSX.Element. Or at least it should have that argument signature.
  • Documentation should show all exported components, the meaning of their arguments, and their purpose.

πŸ“ƒ Provide any additional context for the Bug.

While you're at it, you might want to look at the performance implications of switching from {header({setScreen})} to <Header {...{setScreen}} />. I'm no expert but I think that the current code could cause React to fail to match the header render-to-render, causing unnecessary computation and DOM updates.

This is related to problems which come up in the context of #3246 .

πŸ‘€ Have you spent some time to check if this bug has been raised before?

  • I checked and didn't find similar issue

🏒 Have you read the Contributing Guidelines?

NV-2168

@david-morris david-morris added the polishing Created by Linear-GitHub Sync label Apr 25, 2023
@davidsoderberg davidsoderberg changed the title ✨ Polishing: Fix docs and types for the header [NV-2168] ✨ Polishing: Fix docs and types for the header Apr 25, 2023
@LetItRock
Copy link
Contributor

hey @david-morris! I'm sorry to hear that the docs were not clear about the available props, we are doing our best to improve it, but sometimes something might be missing because it's really evolving part of the code. Thanks for pointing out that we are missing that part.

I think that we can improve the docs as part of the changes in #3286, because it's related to it and any change in the public API should be reflected in the docs.

Speaking about the performance implications of header({setScreen}), there won't be any, unless you return the same React Element (component).

Having the above in mind I'm gonna close this issue if that is ok for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear polishing Created by Linear-GitHub Sync triage
Projects
None yet
Development

No branches or pull requests

2 participants