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.

Sign up for GitHub

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

Jump to bottom

Pm mypackages - cherrypick package details extension #14278

Merged
merged 3 commits into from
Aug 21, 2023
Merged

Pm mypackages - cherrypick package details extension #14278

merged 3 commits into from
Aug 21, 2023

Conversation

dnenov
Copy link
Collaborator

@dnenov dnenov commented Aug 17, 2023

Cherry-pick from #14198 to break it down into smaller chunks.

  • package details extension functionality

Purpose

This is a follow-up PR on the package integration PR. It builds on the continuous design guides as per this Figma link.

Even though a lot of areas have been improved, this is still work in progress. With the main skeleton of the solution in place, the major directions of work are:

  • design changes in Search for packages tab
  • design changes in Publish a package tab
  • design changes in My packages tab
  • design changes in Publish version tab

Screenshots

01 loading screen
01_loading screen

02 discard loading timeout
02_discard loading timeout

03 package details extension interaction
03_package details extension

04 package manager contents
04_package manager contents

05 package manager to preferences interaction
05_package manager to preferences

Declarations

Check these if you believe they are true

  • The codebase 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.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Progression work on the Package Manager

  • interactions between different parts of the package manager and between package manager and preferences screen
  • added empty, loading, idle screens
  • work on the PacakgeDetailsExtension to allow hosting inside the PackageManager
  • misc refactor

Reviewers

@reddyashish
@QilongTang

FYIs

@Amoursol

@QilongTang QilongTang added this to the 3.0 milestone Aug 21, 2023
QilongTang
QilongTang reviewed Aug 21, 2023
View reviewed changes
Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

LGTM with some comments

src/DynamoCoreWpf/Controls/InstalledPackagesControl.xaml Show resolved Hide resolved
src/DynamoCoreWpf/Views/PackageManager/Controls/PackageManagerPackagesControl.xaml.cs Outdated
@@ -89,6 +90,34 @@ private void ViewDetailsButton_OnClick(object sender, RoutedEventArgs e)
if (!(button.DataContext is PackageManagerSearchElementViewModel packageManagerSearchElementViewModel)) return;

var PkgSearchVM = this.DataContext as PackageManagerSearchViewModel;

var name = this.Name;
if (name.Equals(Dynamo.Wpf.Properties.Resources.PackageManagerSearchPackagesControlName))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a common practice to store the name of the control as resources because they will get localized, unless that was your intention? Maybe we can use static readonly strings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes ... this didn't feel right I remember. I replaced them with private read-only static strings now. We cannot set the framework element name to code-behind value, at least I don't think so. Meaning that the values we check still come from two separate places, but I cannot think of a different way of doing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If that's ok, I will remove the redundant localized strings in the next PR, as there are quite a few changes in the resources files lined up.

Copy link
Contributor

@QilongTang QilongTang Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

sure, please do

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the resource with this commit cc58820

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerSearchViewModel.cs Show resolved Hide resolved
src/PackageDetailsViewExtension/PackageDetailsView.xaml.cs Outdated Show resolved Hide resolved
@dnenov
Comments
801528d 
- added comment to the use of 'IsTabStop' inside the InstalledPackagesControl.xaml
- public eventhandler made internal
- framework element name equality check on longer relies on localized strings, using private readonly strings instead
@QilongTang
Copy link
Contributor

No regressions reported. LGTM

@QilongTang QilongTang merged commit d7a505b into DynamoDS:master Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QilongTang QilongTang QilongTang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dnenov @QilongTang