-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
feat(orama): use new UI components #7971
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 so much!
I've left some comments, and I am here to help you implement anything you need help with, just ping me :-).
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aileen Villanueva Lecuona <[email protected]>
Note Your Pull Request seems to be updating Translations of the Node.js Website. Whilst we appreciate your intent; Any Translation update should be done through our Crowdin Project. Thank you! |
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.
I'm still seeing one styling inconsistencies.
I'd love if: @apply
was used when possible, and manual css was only on a if-needed basis.
Additionally, no style
attributes should be needed.
Let me know if you need help with that
.chatLoadingWrapper { | ||
align-items: center; | ||
background-color: #0d121c; | ||
border-radius: var(--radius-m, calc(12rem / var(--orama-base-font-size, 16))); | ||
display: flex; | ||
justify-content: center; | ||
margin: 0 var(--spacing-l, calc(16rem / var(--orama-base-font-size, 16))); | ||
margin-bottom: 24px; | ||
min-height: 60px; | ||
padding: 24px; | ||
} |
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.
Can all of these / most of these be tailwind?
@aileenvl Do you mind if I rebase this + fix some of the formatting to align with our codebase? (I'd need access to edit this PR |
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.
This cannot land until all of the following constraints are met:
- CSS should use Tailwind when possible
- Styles should (almost) never be defined via
style=
- Any/all environment constants should come from
next.constants.mjs
Will address this asap @avivkeller , thank you for the review |
@araujogui Those were unrelated. The (now-removed) blocked label was referring to the unset API keys. |
Yeah, i misunderstood, mb |
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.
Really nice, but the CSS needs some work.
rows={1} | ||
maxLength={500} | ||
autoFocus | ||
className={`${styles.promptFieldCustom} m-0 w-full resize-none border-none bg-transparent p-0 font-normal leading-relaxed text-white outline-none`} |
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.
These should all use the classNames
function
padding: 0; | ||
} | ||
|
||
.chatAssistantMessage p { |
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.
These can be nested
} | ||
|
||
.chatAssistantMessage ul { | ||
@apply list-disc, |
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.
No comma needed. I was incorrect earlier.
This reverts commit 43d8cac.
Preview this PR at https://nodejs-org-git-fork-oramasearch-main-openjs.vercel.app/en
Description
Validation
Related Issues
Check List
pnpm format
to ensure the code follows the style guide.pnpm test
to check if all tests are passing.pnpm build
to check if the website builds without errors.