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

refactor: ♻️ add substance-combobox.tsx #469

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

keinsell
Copy link
Owner

@keinsell keinsell commented May 13, 2024

🚀 This description was created by Ellipsis for commit bb97776

Summary:

Introduced new components (SubstanceCombobox, Combobox, ScrollArea), updated dependencies, and ensured integration and code quality.

Key points:

  • Introduced SubstanceCombobox for framework selection
  • Added Combobox component supporting single and multiple selections
  • Updated dependencies: @radix-ui/react-dialog, @radix-ui/react-popover, cmdk
  • Added ScrollArea component for scrollable areas
  • Integrated SubstanceCombobox into CreateIngestionPage
  • Ensured code quality: no sensitive data logging, adherence to DRY, descriptive naming, and single responsibility principle

Generated with ❤️ by ellipsis.dev

@railway-app railway-app bot temporarily deployed to neuronek-pr-469 May 13, 2024 01:45 Destroyed
Copy link

railway-app bot commented May 13, 2024

🚅 Deployed to the neuronek-pr-469 environment in neuronek

Service Status Web Updated (UTC)
neuronek 😴 Sleeping (View Logs) Web Dec 22, 2024 at 5:12 pm

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 160b913 in 43 seconds

More details
  • Looked at 476 lines of code in 7 files
  • Skipped 1 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_5xvPxTXOurDWBGLY


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

5 days left in your free trial, upgrade for $20/seat/month or contact us.

const [open, setOpen] = useState(false);
const [value, setValue] = useState("");

const frameworks = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SubstanceCombobox component is hardcoded with framework data, which seems incorrect for a component intended to select substances. Consider fetching and using actual substance data here.

@railway-app railway-app bot temporarily deployed to neuronek-pr-469 May 13, 2024 02:03 Destroyed
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on bb97776 in 55 seconds

More details
  • Looked at 289 lines of code in 4 files
  • Skipped 1 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_7HDq6F8HBFe33e2B


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

5 days left in your free trial, upgrade for $20/seat/month or contact us.

return (
<Combobox
mode='single' //single or multiple
options={[]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The options prop is passed an empty array, which means no options will be displayed in the Combobox. You should pass the frameworks array to the options prop to display the intended options.

Suggested change
options={[]}
options={frameworks}

@keinsell
Copy link
Owner Author

Okay... I kinda must spend a lot more time on this, or find another approach for ingestion page.

@keinsell keinsell force-pushed the trunk branch 2 times, most recently from 8f71d72 to 5444560 Compare June 4, 2024 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant