-
Notifications
You must be signed in to change notification settings - Fork 634
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
DYN-6375-Disable-Login #14580
Closed
Closed
DYN-6375-Disable-Login #14580
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
249e524
Enable or not the Sign In button
6a7ee32
Cleaning extra spacing
8d29e11
Changing scope
455701b
Enabling the Signin button as property
095ea6c
Enabling the SignIn button as part of the Ctor
93e16fc
Formating the js object properly
9a7dcbd
Merge branch 'master' into DYN-6375-Disable-Login
QilongTang 21a8c63
Clean up
QilongTang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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