-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
feat(web,design-system,novui): Add embeddable docs #5513
base: next
Are you sure you want to change the base?
Conversation
❌ Deploy Preview for dev-web-novu failed. Why did it fail? →
|
✅ Deploy Preview for novu-design ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Here is the backend service for the docs as well https://github.com/novuhq/cloud-doc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on this, David! The loom videos were super helpful for seeing the whole masterpiece come together and I think this feature will be very helpful for users.
I do have some large-scale feedback that I'm happy to discuss further; part of it may require input from Nik and Dima.
- As mentioned in
AppLayout
, I'm concerned that the context wrapping the whole app could have significant performance implications. I'd like to propose an alternative structure that avoids touching the*Page.tsx
files. Given that all of our documentation is tied to a specific page, we should be able to leverage theROUTES
as a key for a look-up in a configuration file. This would allow us to correlate a route with a documentation path and any other static fields. Furthermore, it provides type-safety and centralizes the related content to minimize touchpoints. We could use a hook to match the current route to a config, and then render the button accordingly. - I strongly propose that we do not put the documentation in the header nav. I don't believe it's a typical pattern, and that bar is already fairly crowded (and may get more so if we choose to add Org and Env switchers there). Instead, I'd propose a FAB (Floating Action Button). They offer more flexibility and extensibility, feel more directly related to the content on the existing page, and seem to match common practices more. I did a quick look, and saw that Linear is using one for the same purpose.
Please let me know if you have any questions about these things or the other feedback in the PR, and again, nice work!
I have now fixed according to your feedback on 1. Number 2 would I love to get @scopsy thoughts on :) |
100% agree with @antonjoel82 on the top navigation bar point. I have already mentioned this earlier this week to nikolay here: https://www.figma.com/design/ybKRCB2ZO5DqICxjxzZGnW?node-id=701-74044#804551101 I would actually go further and say, that the inline documentation should be, well inline 😄 When it's located at the top navbar I would expect it will just open the global docs page for me, as the top navbar is the "global" scope for me personally. Here are a couple of potentially solutions we can iterate over:
Putting this note assigned, I really like how the modal itself and the docs look like! 😍 |
@scopsy @antonjoel82 @davidsoderberg Since we're only testing the functionality --- although the FAB idea is cool and I agree in the long run it would give us more flexibility as a small context menu --- for version one, I'd go with the "icon next to the page title" approach that @scopsy suggested. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work on all the changes, and thanks for walking me through them! There's still a bit of clean-up to be done to help with consistency and to avoid accruing more tech / design debt.
<Alert | ||
className={css({ | ||
borderRadius: '75', | ||
backgroundColor: 'mauve.60.dark !important', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All !important
I have added in latest commit was needed to affect style of their components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤓 nitpick (non-blocking): While using the "new" base color tokens is better than using the legacy.*
tokens, we should still be aiming to use semantic tokens as much as possible in application code so that we can better handle dark/light and in a semantic way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for great improvements on this! My final recommendation would be that this should be behind a feature flag to make it easier to control. Fortunately, it should be fairly straight-forward since we only really have one "entry point."
<Alert | ||
className={css({ | ||
borderRadius: '75', | ||
backgroundColor: 'mauve.60.dark !important', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤓 nitpick (non-blocking): While using the "new" base color tokens is better than using the legacy.*
tokens, we should still be aiming to use semantic tokens as much as possible in application code so that we can better handle dark/light and in a semantic way.
https://www.loom.com/share/ed59a7cc9b834426811994be310e81c6?sid=03dfc7a1-13cf-42c1-8c03-7a31efcf7bae