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

[DYN-4302 + DYN-5259] Add visual indication for deprecated packages #13413

Merged
merged 13 commits into from
Oct 21, 2022

Conversation

zeusongit
Copy link
Contributor

@zeusongit zeusongit commented Oct 20, 2022

Purpose

DYN-4302

  • Disabled package install for deprecated packages from Package Details View Extension and added visual indications for deprecated packages such as tags and tooltips.
  • Also, fixed a bug where user was still able to install packages when Custom Package Locations was disabled from preference settings.
  • Unified UI across Package Manager window and the view extension.
  • Update profile icon to a HD version

Screenshots:

When a package is deprecated:

Screen Shot 2022-10-20 at 1 37 31 AM

When Custom Package Locations is disabled (tooltips are still visible for deprecated packages) :

Screen Shot 2022-10-20 at 1 39 15 AM

Tooltips:

Screen Shot 2022-10-20 at 1 37 41 AM
Screen Shot 2022-10-20 at 1 37 48 AM
Screen Shot 2022-10-20 at 1 37 57 AM

Profile Icon:

Screen Shot 2022-10-20 at 2 52 42 AM
Screen Shot 2022-10-20 at 2 53 59 AM

DYN-5259

  • Fixed horizontal offset for notifications.

Screenshot:

Screen Shot 2022-10-20 at 2 13 35 AM

Screen Shot 2022-10-20 at 2 13 35 AM copy

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

Release Notes

  • Disabled package install for deprecated packages from Package Details View Extension.
  • Added visual indications for deprecated packages such as tags and tooltips.
  • Fixed a bug where user was still able to install packages when Custom Package Locations was disabled from preference settings.
  • Fixed horizontal offset for notifications.

Reviewers

@DynamoDS/dynamo

{
this.PackageName = packageName;
this.PackageVersion = packageVersion;
this.PackageVersionNumber = PackageVersion.version;
this.CopyRightHolder = PackageVersion.copyright_holder;
this.CopyRightYear = PackageVersion.copyright_year;
this.CanInstall = canInstall;

this.IsEnabledForInstall = isEnabledForInstall && canInstall;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use this statement isEnabledForInstall && canInstall in the getter instead so the value is always up to date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean like this:

public bool IsEnabledForInstall
{
    get => isEnabledForInstall && canInstall;
    set
    {
        isEnabledForInstall = value;
        RaisePropertyChanged(nameof(IsEnabledForInstall));
    }
}

and set like:

this.IsEnabledForInstall = isEnabledForInstall;

Copy link
Contributor

Choose a reason for hiding this comment

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

On a second thought, if the setter and getter are pointing to different values, it is also confusing. I guess I am just confused that we have two booleans which are very similar, maybe name them slightly more obvious to each case?

Copy link
Contributor Author

@zeusongit zeusongit Oct 21, 2022

Choose a reason for hiding this comment

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

I actually used the same variable names to be consistent with the PackageManagerSearch window logic,
canInstall handles if the package is already installed and IsEnabledForInstall handles preference settings toggle value.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, let's keep it this way then

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

@QilongTang
Copy link
Contributor

Regression:
DynamoCoreWpfTests.PackageDetailsViewExtensionTests.TestLinksDisplayedInView

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, once conflicts addressed and PR checks pass, good to merge

@mjkkirschner
Copy link
Member

Sorry to comment at the end of this PR and I assume it's already been discussed but why block downloads for deprecated packages? Isn't this annoying if a user needs that package?
For example - nuget does not block downloads for deprecated packages - it does hide them though.

@zeusongit
Copy link
Contributor Author

zeusongit commented Oct 21, 2022

Downloads for deprecated packages was already blocked, what we did in this PR is just to block it in the package details view extension as well. Maybe @Amoursol can help us out here, and continue the discussion forward, if we want to disable downloads for deprecated packages or not.

@QilongTang QilongTang merged commit a1590ca into DynamoDS:master Oct 21, 2022
QilongTang pushed a commit that referenced this pull request Mar 25, 2024
…13413)

* add deprecated label

* update extension

* sync package manger and package view displays

* finishes

* set horizontal offset

* update HD profile icon

* Update PackageDetailsViewModel.cs

* add comment

* conflicts fix

* Update PackageDetailItem.cs

* Revert "conflicts fix"

This reverts commit 2e60462.

* conflict fix2
@QilongTang QilongTang mentioned this pull request Mar 25, 2024
9 tasks
QilongTang added a commit that referenced this pull request Mar 26, 2024
* [DYN-4302 + DYN-5259] Add visual indication for deprecated packages (#13413)

* add deprecated label

* update extension

* sync package manger and package view displays

* finishes

* set horizontal offset

* update HD profile icon

* Update PackageDetailsViewModel.cs

* add comment

* conflicts fix

* Update PackageDetailItem.cs

* Revert "conflicts fix"

This reverts commit 2e60462.

* conflict fix2

* DYN-3652 docs browser Artifakt font fails if not installed (#13654)

* Embedded Artifakt Element as base64

- to ensure the DocumentationBrowser always loads with the correct font, the Artifakt Element font is now included in the css style sheet as base64

* Refactored font resource

- now substitutes an embedded resource containing the font style and converts to base64 before rendering the html page

* Revert unnecessary changes

* FontFace Css rule reintroduced

- brought back a fontface css rule to be whitelisted by the Md2HtmlSanitizer
- all DocumentationBrowserExtension tests passing

* DYN-5297 Import Excel Small Numbers (#13680)

* read scientific notation

* remove unecessary reference

* Fix for situation with formulas and add unit tests for scientific notation

* Dyn 5314 open xml (#13647)

* Fix String display

* Adding Unit test

* Checking Formula

* Fixing Ghost warning (#13820)

* Fixing Ghost warning

* Keeping the Warning Color for the new state

* Adding the Unit test

* Revert "Keeping the Warning Color for the new state"

This reverts commit 40e9643.

* Revert "Fixing Ghost warning"

This reverts commit 04c14c1.

* Updates, Implementation and Test

* fix List.AllIndicesOf node (#13773)

* Handle null values in watch node/preview bubble (#13855)

* Handle null values in watch node/preview bubble

* Add test

* Update WatchNodeTests.cs

* Add null check (#13908)

* DYN-6073 civil3 d packages tour crashing (#14338)

* DYN-6073 Civil3D Packages Guide Crashing

It was crashing due that the user tried to close the Step 4 clicking the PackageSearch window and the Popup is not closed but seems that is already disposed.

* DYN-6073 Civil3D Package Tour Crashing

This change is disabling the close button in the PackageManagerSearch window when running the Packages tour, when passing to the next Step the button is enabled again (unless the next step also requires to disable the button). I've added a new icon image that will be shown when the close button is disabled.
In this way we will be preventing the crash when the user try to close the Packages tour by closing the PackageManagerSearch window

* DYN-6073 Civil3D Package Tour Crashing

This fix will solve the problem of the packages guide crashing when clicking the Library (package installed) for passing from Step8 to Step9.

* Update to remove extra code

* Remove the test causing build failure

* DYN-6123 Fix groups loose associations with notes and nested groups when a custom node is created (#14220)

* Fix note in group in customnode

* add test

* merge test from other PR

* DYN-6130 remove the toast notification (#14317) (#14332)

* Make sure close the Toast notification when Dynamo closes

* Removing extra file

* Removing extra file

* Relocate the responsibility about the toast notification to the DynamoViewModel

* removing unnecessary using

* take advantage of already implementation to apply minimal changes

* Moving the code to a proper place

---------

Co-authored-by: Jesus Alfredo Alviño <[email protected]>

* DependencyType  match (#14314)

* remove extra code

---------

Co-authored-by: Ashish Aggarwal <[email protected]>
Co-authored-by: Deyan Nenov <[email protected]>
Co-authored-by: filipeotero <[email protected]>
Co-authored-by: jesusalvino <[email protected]>
Co-authored-by: aparajit-pratap <[email protected]>
Co-authored-by: reddyashish <[email protected]>
Co-authored-by: Roberto T <[email protected]>
Co-authored-by: Jesus Alfredo Alviño <[email protected]>
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.

3 participants