3D VE implementation & READ workbench rendition integration

[yangli0516]

• Implementation of the 3D VE which displays the 3D model from Sketchfeb. User can create/edit/delete 3D annotations in the 3D VE.
• Integration code for the READ workbench rendition plugin.

[stevewh]

Ciao @yangli0516 , Glad to have you contributing. It's been a while. Wanted to let you know the I started to review this and would like to test the working of it with several of the projects running on my local machine. Can you tell me where to get read-rendition-plugin.js ? Cheers, Steve

[stevewh]

I commented on the stopImmediatePropagation. This needs to be tested as I placed it here to keep other handlers from firing. I can't remember the scenario. The problem is the browser update compared to the service changes to the model.

Third Party javascript should be anchored to a tested implementation, which can be a 3P url which assumes longevity or can be placed into a dependency package for install local to READ.

On Fri, Jun 18, 2021 at 5:25 PM Andrew Glass @.***> wrote:

@.**** commented on this pull request.

In editors/js/editionVE.js https://github.com/readsoftware/read/pull/9#discussion_r654062865:

@@ -3507,7 +3507,7 @@ mergeLine: function (direction,cbError) { ednVE.sclEd.synchSelection(); } }

• e.stopImmediatePropagation();
• // e.stopImmediatePropagation();

@stevewh https://github.com/stevewh what is the impact of allowing immediate propagation. If this is OK, maybe just delete the code rather than comment out.

In index.php https://github.com/readsoftware/read/pull/9#discussion_r654065396:

@@ -59,6 +59,7 @@

@stevewh https://github.com/stevewh what is the right way to include 3p .js files?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/readsoftware/read/pull/9#pullrequestreview-686940028, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARYOIOJXXR4GOBDJKFYAQDTTNQPPANCNFSM46YZRPKQ .

[yangli0516]

Hi @xadxura @stevewh, I've resolved most of the pending issues and comments. Could you please merge the pull request if there's no other issues?

[stevewh]

HI @Yang Li @.***> , thanks for resolving these issues. We are almost there.

Your comments suggest that some of the issues are not resolved, is this true?

Are all the term ids found and converted to lookups (I seem to remember more than a couple but these may have been in the integration code you removed)?

As a side note, please don't think I or Andrew are ignoring you, we are just split between several things and get busy and sometimes distracted). Please don't hesitate to be persistent through email, at least with me.

On Mon, Sep 20, 2021 at 5:50 PM Yang Li @.***> wrote:

Hi @xadxura https://github.com/xadxura @stevewh https://github.com/stevewh, I've resolved most of the pending issues and comments. Could you please merge the pull request if there's no other issues?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/readsoftware/read/pull/9#issuecomment-923487813, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARYOINPTJCOO4LR3EH6DMDUC7JGHANCNFSM46YZRPKQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[yangli0516]

Hi @stevewh , have added in additional comments that resolutions are clear on all issues. I think we are good for merge.

[xadxura]

Looks good to me. Thank you Yang!

[stevewh]

Also to me. Thanks to both of you for taking the time to process this PR.

On Thu, Sep 23, 2021 at 6:48 PM Andrew Glass @.***> wrote:

Looks good to me. Thank you Yang!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/readsoftware/read/pull/9#issuecomment-925984306, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARYOIIG5QUCBTZ6Z6ECCLTUDNK4TANCNFSM46YZRPKQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.