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

DYN-1175: Captured image name default to "graph name+timestamp". #9546

Merged
merged 4 commits into from
Mar 8, 2019
Merged

DYN-1175: Captured image name default to "graph name+timestamp". #9546

merged 4 commits into from
Mar 8, 2019

Conversation

reddyashish
Copy link
Contributor

Purpose

This PR addresses a minor improvement when the user tries to capture an image of the workspace or the Background 3d image snapshot.

A user can take a snapshot in the following ways:

  1. File => Export Workspace As Image
  2. File => Export Background 3D Preview As Image
  3. Camera Icon (Top Right) while workspace is active
  4. Camera Icon while background preview is active

The snapshot name will default to the graph name along with the timestamp.

snapshot name

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.

Reviewers

@QilongTang @mjkkirschner

@reddyashish
DYN-1175
c2de466 
Captured image name default to "graph name+timestamp".
@reddyashish
DYN1175
d00563b 
Added comments
@QilongTang
Copy link
Contributor

One more comment and then LGTM

@QilongTang QilongTang added the LGTM Looks good to me label Mar 6, 2019
@QilongTang QilongTang self-assigned this Mar 6, 2019
aparajit-pratap
aparajit-pratap approved these changes Mar 7, 2019
View reviewed changes
Copy link
Contributor

@aparajit-pratap aparajit-pratap left a comment

Choose a reason for hiding this comment

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

Aside from the name change suggestion, LGTM.

src/DynamoUtilities/PathHelper.cs Outdated Show resolved Hide resolved
@QilongTang
Copy link
Contributor

I looked at the self CI results, this PR should be safe to merge. We do need to fix the self CI ASAP

@QilongTang QilongTang merged commit 88c925e into DynamoDS:master Mar 8, 2019
reddyashish added a commit to reddyashish/Dynamo that referenced this pull request Mar 13, 2019
@reddyashish
DYN-1175: Captured image name default to "graph name+timestamp". (Dyn…
3d4d0c1 
…amoDS#9546)

* DYN-1175

Captured image name default to "graph name+timestamp".

* DYN1175

Added comments

* Function API changes

* Changing API name

(cherry picked from commit 88c925e)
@reddyashish reddyashish mentioned this pull request Mar 13, 2019
Merged
7 tasks
reddyashish added a commit that referenced this pull request Mar 13, 2019
@reddyashish
DYN-1175: Captured image name default to "graph name+timestamp". (#9546…
a30eb27 
…) (#9565)

* DYN-1175

Captured image name default to "graph name+timestamp".

* DYN1175

Added comments

* Function API changes

* Changing API name

(cherry picked from commit 88c925e)
@johnpierson
Copy link
Member

fixes #2695

@reddyashish reddyashish deleted the DYN1175 branch November 23, 2022 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QilongTang QilongTang QilongTang left review comments

@aparajit-pratap aparajit-pratap aparajit-pratap approved these changes

Assignees

@QilongTang QilongTang

Labels
LGTM Looks good to me
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@reddyashish @QilongTang @johnpierson @aparajit-pratap