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(frontend) Refactor and move components #5290

Merged
merged 16 commits into from
Dec 2, 2024

Conversation

amanape
Copy link
Member

@amanape amanape commented Nov 26, 2024

The objective of this PR is to

  • break down large components (approximately > 150 lines) into smaller ones,
  • move components into appropriate directories

Components are organized into folders based on their domain, feature, or shared functionality.

components
├── features # Domain-specific components
├── layout
└── shared # Shared UI components
  • Added custom base-button to TailwindCSS for common button styles

This PR does not break down or merge some of the modal components as much as I initially planned. I hope to address this soon by implementing the Compound Component Pattern, which should provide a more convenient and flexible way to build them.


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:e31a1fd-nikolaik   --name openhands-app-e31a1fd   docker.all-hands.dev/all-hands-ai/openhands:e31a1fd

@amanape amanape self-assigned this Nov 26, 2024
@amanape amanape marked this pull request as ready for review November 27, 2024 17:21
Copy link
Collaborator

@rbren rbren left a comment

Choose a reason for hiding this comment

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

There's a LOT here--is there an easy way to break it down into smaller PRs?

I assume this is all just refactoring and no functional changes--if you're confident w/ it I'm happy to approve but it's a lot to review 😅

@amanape
Copy link
Member Author

amanape commented Nov 27, 2024

Yes it is a lot, I recommend you switch to the branch on your local IDE so the changes will become clear.

What I've done:

  1. Break down components into smaller ones (e.g., jupyter.tsx into jupyter.tsx, jupyter-cell.tsx, etc...)
  2. Organize them into folders (features/jupyter/jupyter.tsx, features/jupyter/jupyter-cell.tsx, etc...)
  3. Update imports

There is no change in behaviour or logic, only reducing component size/complexity, removing duplicates (e.g., two different loading-spinner.tsx, moving some logic into functions and into utils (e.g., parse-cell-content.ts)

@amanape
Copy link
Member Author

amanape commented Nov 27, 2024

Sorry, I know it is a lot but the alternative would have been ~10-20 different PRs for breaking down and/or moving each component. Since there is no behaviour/logic changes, I thought I'd just submit it at once. It is more of a "look at the IDE file tree PR" rather than testing features

Copy link
Collaborator

@rbren rbren left a comment

Choose a reason for hiding this comment

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

sounds like this is mostly just copy/paste, so the diff isn't as scary as it looks. 🍰

@amanape amanape enabled auto-merge (squash) December 2, 2024 05:30
@amanape amanape merged commit b9b6cfd into main Dec 2, 2024
13 checks passed
@amanape amanape deleted the ALL-796/refactor-components branch December 2, 2024 05:47
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.

3 participants