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

Issues/passing metaverse url login register #43

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

pankaj2k9
Copy link
Contributor

No description provided.

@Misterblue Misterblue self-requested a review January 31, 2022 16:45
@Misterblue
Copy link
Contributor

I presume this is addressing Issue #24. I see how the metaverse URL is passed down from the login dialog and through all the layers (dialog => Account.login => buildURL) but it isn't integrated with the global settings. For instance, if the app was connected to metaverse X and then did a login for and account in metaverse Y, the global logged-in-account (Account.) variables will be set to this new account but the metaverse info would be lost (the Account. doesn't store the metaverseURL and MetaverseMgr.ActiveMetaverse will be pointing to a different metaverse.
The current design has one connected metaverse-server and one logged in account and this information is kept in the app's environment (and Vue's store). If there are going to be multiple metaverses and multiple logged in accounts, there needs to be a "Logged in account" class and the variables kept separately.

@digisomni digisomni added the enhancement New feature or request label Feb 2, 2022
Copy link
Contributor

@Misterblue Misterblue left a comment

Choose a reason for hiding this comment

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

There are several calls to Utility.connectionSetup() which takes a domain URL. Since this code is setting a metaverse-server URL, these should be calls to Utility.metaverseConnectionSetup(). Also, the code explicitly sets the value of $Store.metaverse.server when there is a more general mutation of StoreActions.UPDATE_METAVERSE. It's bad practice to do direct store updating when a mutation or action should be defined so the next person knows what operations happen on $Store.

src/components/components/login/MetaverseLogin.vue Outdated Show resolved Hide resolved
src/components/components/login/MetaverseLogin.vue Outdated Show resolved Hide resolved
src/components/components/login/MetaverseRegister.vue Outdated Show resolved Hide resolved
src/components/components/login/MetaverseRegister.vue Outdated Show resolved Hide resolved
src/components/components/login/MetaverseRegister.vue Outdated Show resolved Hide resolved
src/components/components/login/MetaverseRegister.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@Misterblue Misterblue left a comment

Choose a reason for hiding this comment

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

The addition of the optional metaverseURL to some functions is confusing/wrong.
Why remove the explicit use of the BABYLON prefix to functions and variables. Since some graphics names are reused, it is often nice to know one is using the BABYLON.Vector3 rather than another package's definition of a Vector3. Just a style question and not a functional question.

src/modules/account/index.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants