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

Issue 9577 - Text can be typed whilst the package manager syncs with the server. #9583

Merged
merged 8 commits into from Apr 17, 2019

Conversation

RyaPorter
Copy link
Contributor

Purpose

Fixes Issue 9577. The search text box is now collapsed until the package manager has finished syncing with the server. After this the search box functions as before.

PackageSearchFix

PackageSearchFix_AfterSync

PackageSearchFix_AfterType

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

GLOBAL\ryan.porter added 3 commits March 18, 2019 09:43
Added styles to control package search information text.
…d. Both text boxes now have transparent backgrounds, caret brush set to white (otherwise it's black).
@mjkkirschner
Copy link
Member

Thanks for this work @RyaPorter - this seems like functionality that should have a test which verifies the correct properties for the text box given a modified state.

GLOBAL\ryan.porter added 3 commits March 21, 2019 12:39
…o control component behaviour.

Added old visibility converter to collpse search box prompt when text is being typed.
@RyaPorter
Copy link
Contributor Author

Thanks @mjkkirschner . I've made some changes as suggested. Instead of using styles, the behaviour of the two text boxes is now controlled by properties in the view model and appropriate tests have been added to test these properties.

@@ -156,6 +183,8 @@ public PackageSearchState SearchState
{
_searchState = value;
RaisePropertyChanged("SearchState");
RaisePropertyChanged("SearchBoxPrompt");
Copy link
Member

Choose a reason for hiding this comment

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

you should be able to use nameof here

@mjkkirschner
Copy link
Member

Thanks for pushing this forward, and adding the tests, just a few more requests.

@RyaPorter
Copy link
Contributor Author

Cheers, I've made the changes now.

@@ -145,17 +181,21 @@ public bool HasNoResults
get { return this.SearchResults.Count == 0; }
}


public PackageSearchState _searchState;
Copy link
Contributor

Choose a reason for hiding this comment

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

@RyaPorter Can you add a comment suggesting that this backing field be set to private in Dynamo 3.0? Doing so now would unfortunately introduce an API breaking change.

@alfarok
Copy link
Contributor

alfarok commented Apr 16, 2019

Hey @RyaPorter, I add one comment suggesting we add a comment for future reference regarding a public/private assessor. Otherwise, I pulled down this branch for some local testing and everything seems to be working as expected. I also ran the branch on the self-serve CI verifying all new/existing tests successfully pass. @mjkkirschner @QilongTang I believe this should be good to go otherwise.

@RyaPorter
Copy link
Contributor Author

@alfarok I've added that comment. Also added comments to some other public backing fields I spotted.

@alfarok
Copy link
Contributor

alfarok commented Apr 17, 2019

Thanks @RyaPorter, LGTM merging 👍

@alfarok alfarok merged commit c18e6df into DynamoDS:master Apr 17, 2019
mjkkirschner pushed a commit that referenced this pull request Aug 3, 2019
…the server. (#9583)

* Added "Please wait..." string to resource files.

Added styles to control package search information text.

* Applied styles to text boxes.

* Swapped grey background colour from search text box to search box grid. Both text boxes now have transparent backgrounds, caret brush set to white (otherwise it's black).

* Removed styles and add testable properties in the viewmodel instead to control component behaviour.
Added old visibility converter to collpse search box prompt when text is being typed.

* Renamed string resource for "Please Wait..." to be a little bit more generic.

* Added unit tests to test search box behaviour based on different search states.

* Added property summaries. Replaced property name strings with nameof

* Added comments on non-private backing fields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minor UI Bug - Text can be typed whilst the package manager syncs with the server.
3 participants