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

App Store Drawer #636

Merged
merged 27 commits into from May 16, 2023
Merged

Conversation

aozien
Copy link
Collaborator

@aozien aozien commented Feb 26, 2023

This PR should close #464

To get this to work you've to first serve this file from visual studio, so that it's served on port 5500, you can also change the AppStoreData.json file to change the action to be any url (update: now it's served from local path)

How storing the file data works:

1- a new property loadedFileInfo is added to the store, and gets updated when the model changes.
2- if the model was loaded from a local file we don't have to store the whole file, storing the file info is enough, and the hosted frame will be able to re-read the file from disk.
3- if the model was loaded from bldrs/share or github it will store the

How sharing the data with the app works:

Currently this implementation uses the native MessageChannel to avoid any overheads or dependencies, so that apps can integrate without having to depend on certain libraries, but that's a topic that can be open for discussion as well

The hosted frame sends an action name, and the host will respond with the same action name with the response data

image

And for local files
image

@netlify
Copy link

netlify bot commented Feb 26, 2023

Deploy Preview for bldrs-share ready!

Name Link
🔨 Latest commit 6be26a5
🔍 Latest deploy log https://app.netlify.com/sites/bldrs-share/deploys/6462272fbac6ae00087ddb0b
😎 Deploy Preview https://deploy-preview-636--bldrs-share.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@pablo-mayrgundter
Copy link
Member

wowow!

image

@pablo-mayrgundter
Copy link
Member

Could you please move the AppStore button into the third group, after the separator and before the Theme button.

There's a typo in the Vyzn App description "environemental" -> "environmental". Could you please also add to the description smth like "- DRAFT"

Also, see the screenshot.. the icon is filled in black. Is that as intended? I think something more like this? https://thenounproject.com/icon/apps-5361891/

@OlegMoshkovich thoughts?

@pablo-mayrgundter
Copy link
Member

@oo-bldrs could you check this out from a security perspective? The message submission from form input makes me a little nervous but it's been ages since i've thought about code injection

@pablo-mayrgundter
Copy link
Member

Might be nice to have the Vyzn App icon from the Store prefix the title when the App is active. Wdyt?

image

@aozien
Copy link
Collaborator Author

aozien commented Mar 8, 2023

@pablo-mayrgundter I believe it makes sense to add the app icon as well, I've updated the branch
Regarding the Icon, it was just a placeholder, also I'm not sure if I can use the icon you provided now because of its license, so I used the closest one that I could find, let me know what you think.

@pablo-mayrgundter
Copy link
Member

This looks good. Not sure we want it live yet though. Talked with @Adrian62D today and one idea is to add a feature flag to the URL, e.g. feature=apps, and set it in the zustand store on init. So launch process would be:

  • That way you can check in code now, disabled in zustand by default
  • enabled if feature flag present in URL for testing
  • Later enabled by default in zustand, optionally removed

@aozien
Copy link
Collaborator Author

aozien commented Mar 20, 2023

Hi @pablo-mayrgundter , I've updated the branch from main, and added the feature=apps check before displaying the open app store icon, PTAL :)

Copy link
Member

@pablo-mayrgundter pablo-mayrgundter left a comment

Choose a reason for hiding this comment

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

Ok, I think this is the last set of changes. Just some code organization.

src/BaseRoutes.jsx Outdated Show resolved Hide resolved
@pablo-mayrgundter
Copy link
Member

Let's have some more planning here.. https://discord.com/channels/853953158560743424/1090001703681658890/1098247117685932193

If we can't resolve it, let's get this merged.

@pablo-mayrgundter pablo-mayrgundter self-assigned this Apr 19, 2023
@pablo-mayrgundter
Copy link
Member

I had lost track of this.. @aozien you had a blocking question there. Let's try in the future to make sure any blocking issues are mentioned here.

This is me doing that now

TODO(pablo): answer Zien here:
https://discord.com/channels/853953158560743424/1090001703681658890/1098247117685932193

@pablo-mayrgundter
Copy link
Member

I had lost track of this.. @aozien you had a blocking question there. Let's try in the future to make sure any blocking issues are mentioned here.

This is me doing that now

TODO(pablo): answer Zien here: https://discord.com/channels/853953158560743424/1090001703681658890/1098247117685932193

Hey Zien, I just replied in the discord...

"Ok. It was a useful exercise to clean up the initial handling but I can see this being too brittle to rely on across a long session.

However, this discussion stemmed from cypress testing. Now that the feature flag isn't cleared immediately, can you finish the cypress tests?"

If we can get this resolved I think this PR is ready.

Copy link
Member

@pablo-mayrgundter pablo-mayrgundter left a comment

Choose a reason for hiding this comment

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

Awesome!

src/BaseRoutes.jsx Outdated Show resolved Hide resolved
@pablo-mayrgundter pablo-mayrgundter merged commit c4cb51a into bldrs-ai:main May 16, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Closed PRs
Development

Successfully merging this pull request may close these issues.

VYZN: As a user I can launch the vyzn app from Bldrs
2 participants