Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

Open brush integration #20

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Open brush integration #20

wants to merge 16 commits into from

Conversation

andybak
Copy link

@andybak andybak commented Nov 21, 2023

  1. Initial simple search functionality
  2. Remove category search placeholder
  3. Thumbnail view/upload support for assets
  4. Generate a device code to enable login from within Open Brush
  5. Update the poly assets browser to use poly.pizza instead via our wrapper API (Not part of the main nav - more of a test at the moment. Might hide this entirely)

@andybak
Copy link
Author

andybak commented Nov 21, 2023

@foxtrotluna

@@ -3,3 +3,5 @@ node_modules/
.env
.env.dev
.env.live
build
.idea/*
Copy link
Contributor

@foxtrotluna foxtrotluna Nov 29, 2023

Choose a reason for hiding this comment

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

Nitpick, but .idea/* and other IDE related config ignores should be in your local global gitignore file IMO. Happy to leave this, but most places I've worked only have gitignore ignore files that might be generated/required by the app specifically.

Copy link
Author

Choose a reason for hiding this comment

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

Fair point. It's good practice and is probably something I should be doing anyway.

@foxtrotluna
Copy link
Contributor

Sorry I've taken a while to get around to this.

First thing I've noticed, getting SSL errors due to a security vulnerability fixed in node 17. See solution. I can run it by downgrading my node version, but we probably should consider whether we want to upgrade to compatibility with the latest LTS instead.

@foxtrotluna
Copy link
Contributor

I haven't had a chance to properly poke at the code yet, but search doesn't appear to work at all for me. Is it searching by name, description, ID? It looks like it's calling the API with the search, are there API updates too or is the one at https://api.icosa.gallery/ on the latest?

@andybak
Copy link
Author

andybak commented Nov 30, 2023

I haven't had a chance to properly poke at the code yet, but search doesn't appear to work at all for me. Is it searching by name, description, ID? It looks like it's calling the API with the search, are there API updates too or is the one at https://api.icosa.gallery/ on the latest?

Yep. There's matching changes on a branch for the back-end too. I've been running it locally but I guess we could deploy it. @mikeskydev ?

@andybak
Copy link
Author

andybak commented Nov 30, 2023

Sorry I've taken a while to get around to this.

First thing I've noticed, getting SSL errors due to a security vulnerability fixed in node 17. See solution. I can run it by downgrading my node version, but we probably should consider whether we want to upgrade to compatibility with the latest LTS instead.

Getting this building took me almost as long as writing the code. I get completely lost with node and package version issues (which is quite a statement coming from a Python dev!) so I was wary of changing anything.

Is this issue a blocker? What's the impact?

@foxtrotluna
Copy link
Contributor

Sorry I've taken a while to get around to this.
First thing I've noticed, getting SSL errors due to a security vulnerability fixed in node 17. See solution. I can run it by downgrading my node version, but we probably should consider whether we want to upgrade to compatibility with the latest LTS instead.

Getting this building took me almost as long as writing the code. I get completely lost with node and package version issues (which is quite a statement coming from a Python dev!) so I was wary of changing anything.

Is this issue a blocker? What's the impact?

It’s not a blocker per se, though newer versions of node will refuse to run it so we’ll have to stick with 16. I wouldn’t say it would block the merge but it is an issue to be addressed soon for certain.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants