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

Pm publish a package - numeric up/down control #14374

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

dnenov
Copy link
Collaborator

@dnenov dnenov commented Sep 5, 2023

Purpose

This is a short PR with some of the UI changes to the 'Publish' tab in the package manager. The work in this PR focuses around the updated numeric input control for package versions.

UI Changes

numerical input - positive numbers

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

  • Updated numeric input: free input or use carets to increase/decrease number
  • used and restyled the IntegerUpDown control from DotNetProjects.Extended.Wpf.Toolkit, which is already part of DynamoCoreWpf
  • updated existing resources, added new label resources

Reviewers

@QilongTang
@reddyashish

FYIs

@Amoursol

- Updated numeric input: free input or use carets to increase/decrease number
@QilongTang QilongTang added this to the 3.0 milestone Sep 5, 2023
@@ -2190,7 +2199,7 @@ Uninstall the following packages: {0}?</value>
<value>Version {0} of {1} could not be found.</value>
</data>
<data name="PublishPackageViewPackageHostDependency" xml:space="preserve">
<value>External Dependency (optional)</value>
<value>Host (optional)</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to revisit this with @hwahlstrom again. I feel this change would eliminate the chance of describing a Dynamo package depending on external software existence rather than integration or host. I think the term Host is very limited to the current way Dynamo is integrated, so we did not pick this term in the first place.

But I guess @hwahlstrom 's starting point is that external dependency may not be trivial enough for users to understand in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

I hope we can find the bandwidth to rethink this system - there was a pretty good discussion originally and we discarded some flexible ideas because of time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@QilongTang Do you mean the change of terminology from External Dependency to Host? The drop-down only has hosts. If we want a way to introduce dependencies other than hosts, I'm happy to think that through. However, if we are only offering hosts, let's use the simpler term that more users will understand.

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.

One comment then LGTM

@dnenov dnenov marked this pull request as draft September 6, 2023 11:53
- replaced the xctk:IntegerUpDown with native wpf custom user control to reduce reliance on external dependencies
@dnenov dnenov marked this pull request as ready for review September 6, 2023 16:17
- now only allows positive whole numbers
@QilongTang QilongTang merged commit 3590125 into DynamoDS:master Sep 7, 2023
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.

4 participants