DYN-6375-Disable-Login #14580
DYN-6375-Disable-Login #14580
Conversation
|
Can you attach a screenshot/gif of the change? |
Sign In enabled / disabled
|
| /// Enable or not the SignIn button on the fly. | ||
| /// </summary> | ||
| /// <param name="enabled"></param> | ||
| internal async void SetSignInEnable(bool enabled) |
There was a problem hiding this comment.
You may need to expose this as an API and provide an example of how to use it as part of this PR
There was a problem hiding this comment.
You may need to expose this as an API and provide an example of how to use it as part of this PR
Sure, currently is just a method of the SplashScreen class, Do you want to extend it as part of the DynamoViewModel, like the RequestClose FYI @reddyashish ?
There was a problem hiding this comment.
Please think about it in the integration context: https://github.com/DynamoDS/DynamoRevit/blob/master/src/DynamoRevit/DynamoRevit.cs#L334 how to disable it when trying to show it from integrations.
There was a problem hiding this comment.
You may need to expose this as an API and provide an example of how to use it as part of this PR
@QilongTang @mjkkirschner @reddyashish , changing it to public is enough to expose it so can be called from other parts. Unfortunatelly after varioous tests on D4R deptly I have realized its method OnRequestDynamicSplashScreen is called before the splash creen is ready and it's being used by Revit on that way which IMHO is a bug because the splash screen is ready only when its web resources are completed loaded and ready to have access to the WV2's DOM. That's why an example over the current implementation of the flow between Dynamo and D4R doesn't work. Please let me know What Do you consider to complete this PR, thanks.
There was a problem hiding this comment.
Why are we sticking to a method, not some startup config for SplashScreen constructor? A method call is meant to fail after UI initialized?
There was a problem hiding this comment.
Why are we sticking to a method, not some startup config for SplashScreen constructor? A method call is meant to fail after UI initialized?
A method approach means the consumers can call it on demand / Yes because the consumers don't know when the UI is initialized properly (image above).
I have moved the responsibility to enable or not the SignIn button to the ctor so the consumers will be able to decide only at that time.
Now the Splashcreen is the responsible to enable or not the SignIn button only when the web resources are completely loaded.
The ctor will enable the SignIn button by default, otherwise it needs to set explicitly here https://github.com/DynamoDS/DynamoRevit/blob/master/src/DynamoRevit/DynamoRevit.cs#L334 according to the consumer's criteria.
Enabled
Disabled
|
IMO this should not be an API - Dynamo should determine this, putting all these considerations on integrators just makes their jobs harder for what benefit? Dynamo knows if the user is logged in or not - is that correct? Perhaps I'm misunderstanding when the signIn button should be enabled and when it shouldn't be. In general my feeling is that the splash screen is a burden for clients to integrate and we should be doing what we can to make it simpler, not more complex. |
@mjkkirschner about the benefit and your misunderstanding, kindly I guess is part of the Dynamo's strategy to have this feature, That's up to you guys @QilongTang Yup, Dynamo knows if the user is logged in or not, that's already handled as part of the second AC of this task. IMHO exposing this feature as part of its API doesn't mean mandatory for the consumer, just let me know your decision guys. |
|
Closing in favor of #14653 |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
Purpose
Adding an option to the Splash Screen API to enable / disable the Sign in button
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@QilongTang
@reddyashish
FYIs
@RobertGlobant20
@Enzo707