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

VYZN: Viewer: UI configuration via Matrix config message #525

Merged
merged 42 commits into from Feb 1, 2023

Conversation

Ibrahim5aad
Copy link
Collaborator

@Ibrahim5aad Ibrahim5aad commented Dec 21, 2022

This PR refactors the underlying classes for the widget API communication mechanism that allows the Share app to be treated as an iframe-embeddable app that supports receiving and sending messages back and forth with the embedding apps.

The main theme for this PR is supporting #391 UI configuration via this previously mentioned widget API mechanism.

The PR also stabilizes the e2e tests for this API integration and adds the required test for the new functionality UI configuration.

09/01 Update: Closes #391, closes #515, closes #516, closes #517.

@netlify
Copy link

netlify bot commented Dec 21, 2022

Deploy Preview for bldrs-share ready!

Name Link
🔨 Latest commit e8e7a9b
🔍 Latest deploy log https://app.netlify.com/sites/bldrs-share/deploys/63d987d2b9ebb80009c7f736
😎 Deploy Preview https://deploy-preview-525--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.

@Ibrahim5aad
Copy link
Collaborator Author

It's weird that the load-sample-model e2e test pass for me locally, but "sometimes" fail when triggered by the github action.

@pablo-mayrgundter
Copy link
Member

It's weird that the load-sample-model e2e test pass for me locally, but "sometimes" fail when triggered by the github action.

Hi Ibrahim! Looks good overall. We have a fix underway for the e2e test.. will try to get it in tomorrow. Hopefully that fixes the issue for you too.

One high-level comment on the UI config. I'm not sure it makes sense to have individual button enable toggles. Do you guys have use-cases for that? I'll go back to the bug for more discussion...

@Ibrahim5aad Ibrahim5aad changed the title Viewer: UI configuration via Matrix config message #391 Viewer: UI configuration via Matrix config message Dec 26, 2022
@Ibrahim5aad
Copy link
Collaborator Author

@pablo-mayrgundter Hi Pablo! I modified the implementation to be more coarse in hiding/showing UI components. (per button group). Is this making more sense now?

@OlegMoshkovich OlegMoshkovich changed the title Viewer: UI configuration via Matrix config message VYZN: Viewer: UI configuration via Matrix config message Jan 10, 2023
@Ibrahim5aad Ibrahim5aad deleted the branch bldrs-ai:main January 11, 2023 16:55
@Ibrahim5aad Ibrahim5aad deleted the main branch January 11, 2023 16:55
@Ibrahim5aad Ibrahim5aad restored the main branch January 11, 2023 16:56
@Ibrahim5aad Ibrahim5aad reopened this Jan 11, 2023
@pablo-mayrgundter
Copy link
Member

Hi Ibrahim! Thanks for waiting to get #455 in. Could you merge and we'll go from there? Thanks.

@pablo-mayrgundter Hi Pablo! I modified the implementation to be more coarse in hiding/showing UI components. (per button group). Is this making more sense now?

Yes, great, thanks!

@OlegMoshkovich
Copy link
Member

@Ibrahim5aad can you please join our discord?
https://discord.gg/BxHA5TBe

@Ibrahim5aad
Copy link
Collaborator Author

Hi @pablo-mayrgundter, conflicts are fixed, and this PR is ready to get through now.

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, looking pretty good. One refactor in Utils.

Also, let's avoid including Momentum (5MB) as a test asset. Can you use a constructed IFC to get what you need? Here's a minimal IFC file to start with:

https://discord.com/channels/853953158560743424/945373774402945074/1048365704077910096

src/WidgetApi/Utils.js Outdated Show resolved Hide resolved
@pablo-mayrgundter pablo-mayrgundter merged commit c7f97db into bldrs-ai:main Feb 1, 2023
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: Completed
5 participants