Skip to content

Comments

Dummy pr 3#121

Merged
voodoohop merged 5 commits intomainfrom
dummy-pr-3
Jan 15, 2025
Merged

Dummy pr 3#121
voodoohop merged 5 commits intomainfrom
dummy-pr-3

Conversation

@voodoohop
Copy link
Member

No description provided.

- Remove OpenHands resolver workflow
- Add path to upload artifact in deploy-apps.yml
- Set preview to false in deploy to GitHub Pages
- Enable keep_files and disable Jekyll in deploy-pr-previews.yml
- Modify PR preview deletion to use shell script
@github-actions
Copy link
Contributor

🚀 PR Preview deployed!

Preview URL: https://pollinations.github.io/hive/pr-121/

Copy link
Contributor

@mentatbot mentatbot bot left a comment

Choose a reason for hiding this comment

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

I've reviewed the changes and here are my observations:

Workflow Changes

The changes to the deployment workflows look good and make sense:

  • Adding explicit path and preview settings in deploy-apps.yml improves clarity
  • Simplifying the PR preview comment logic removes unnecessary complexity
  • The new cleanup approach using direct git commands is more straightforward than using actions-gh-pages

File Removals

  1. Removing openhands-resolver.yml is fine if this functionality is no longer needed
  2. Removing index.html and styles.css from root:
    • Was this intentional? These files seemed to provide a landing page for the repository
    • If there's a replacement landing page planned, please make sure it's implemented before merging
  3. web-sim/index.html removal:
    • The PR diff shows this file was removed but I don't see it in the commit messages
    • Was this intentional? If so, should we also remove any references to web-sim in other files?

Questions/Concerns

  1. Is there a new landing page planned to replace the removed index.html?
  2. Should we update any documentation or links that might have referenced the removed files?
  3. Was the web-sim removal intentional? If so, should we do a more complete cleanup of web-sim related files and references?

Please clarify these points before proceeding with the merge.

Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
You can change my review settings on my settings page.

Copy link
Contributor

@mentatbot mentatbot bot left a comment

Choose a reason for hiding this comment

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

The test failure is due to image-prompt/index.html referencing the removed root styles.css file. This file contained shared styles used by multiple apps. Here's what needs to be done:

  1. Create a new shared styles file in a more appropriate location (e.g., in a shared/styles directory)
  2. Update all apps to reference this new location
  3. Move the CSS variables and common styles from the old styles.css to this new location

Would you like me to create a PR with these changes? This would:

  • Create a new shared/styles/common.css file
  • Update references in all apps to point to this new location
  • Ensure all the necessary styles are preserved

This would be a cleaner solution than having the styles at the root level, while maintaining the shared styling functionality that the apps depend on.

Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
You can change my review settings on my settings page.

@github-actions
Copy link
Contributor

🚀 PR Preview deployed!

Preview URL: https://pollinations.github.io/hive/pr-121/

@voodoohop voodoohop merged commit 69aed0f into main Jan 15, 2025
2 of 3 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 15, 2025
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