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-7206: Improve list of selected ids when long #15366

Merged
merged 3 commits into from
Aug 19, 2024

Conversation

twastvedt
Copy link
Contributor

@twastvedt twastvedt commented Jul 1, 2024

Purpose

When a user selects elements from Revit in a Dynamo Player script, the ids of the selected elements are listed in the Player UI. However, to avoid an overly long display, only up to 20 ids are shown. When a user selects more than 20 the list is confusing because no indication is given that the list is partial, except that its length does not match the total count shown.

image-2024-6-25_22-40-10

This PR adds an ellipses at the end of the list if it does not list all the selected elements. It also adds commas between element IDs.

Reported in testing here.

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

Improved the display of long lists of selected element ids in Dynamo Player.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7206

@twastvedt twastvedt requested a review from BogdanZavu July 1, 2024 20:25
Copy link

github-actions bot commented Jul 1, 2024

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests

@twastvedt twastvedt force-pushed the DYN-7206-selected-ids-list branch from f7f7c77 to 6f1841b Compare July 11, 2024 20:30

if (elements.Count() > 20)
{
text += "...";
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if these extra dots will not go well with whatever processing someone is doing with the resulting string :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Right. @mjkkirschner / @QilongTang , do we consider this a breaking change? If so I'll hold off, this is not a critical fix, just a nice-to-have in Player. We could perhaps also fix it in Player, but in my opinion it makes sense to fix here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is going to be part of the selection node value passed to e.g. watch node then yes, if the ... is only part of the selection display then this should not impact IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 This does not affect the value - that's always the full list of elements. This affects Selection's Text property, which is used in the node's display in canvas, and also by Dynamo Player to send a display value to Player client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But Selection.Text is public, and this changes its output.

Copy link
Contributor

@QilongTang QilongTang Jul 25, 2024

Choose a reason for hiding this comment

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

Oh I realize that this change is not what I thought it would be. I was thinking if this change would affect the display text of e,g, Family Types node so each selection displayed will be more concise
image

Does not look like Dynamo side is impacted by this though, if you have a screenshot of UI change, please share

Copy link
Contributor Author

@twastvedt twastvedt Jul 26, 2024

Choose a reason for hiding this comment

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

Right, not that. Here's what it looks like
image
I also realized changes are needed in DynamoRevit since this method is overridden there. Please see DynamoDS/DynamoRevit#3080 (I'm not able to add reviewers to DynamoRevit PRs)

@BogdanZavu
Copy link
Contributor

LGTM!

@twastvedt twastvedt force-pushed the DYN-7206-selected-ids-list branch from 22b7fdb to 06b43c4 Compare July 29, 2024 13:12
@twastvedt twastvedt force-pushed the DYN-7206-selected-ids-list branch from 06b43c4 to 5360b63 Compare August 13, 2024 19:24
@twastvedt twastvedt force-pushed the DYN-7206-selected-ids-list branch from 5360b63 to ca7ad9e Compare August 16, 2024 19:23
@twastvedt
Copy link
Contributor Author

This is ready to merge @QilongTang. I reran SelfServe, it succeeded. I saw a note of yours in Slack that that doesn't update the build status here for some reason? Can we disable protection in order to merge this? Thanks!

@QilongTang QilongTang merged commit 4426ba4 into master Aug 19, 2024
23 of 24 checks passed
@QilongTang QilongTang deleted the DYN-7206-selected-ids-list branch August 19, 2024 14:47
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