Skip to content

368: Use Local File Path annotation#370

Merged
BrianWhitneyAI merged 13 commits intofeature/local_cloud_togglefrom
feature/368-use-local-file-path-annotation
Jan 2, 2025
Merged

368: Use Local File Path annotation#370
BrianWhitneyAI merged 13 commits intofeature/local_cloud_togglefrom
feature/368-use-local-file-path-annotation

Conversation

@pgarrison
Copy link
Copy Markdown
Contributor

@pgarrison pgarrison commented Dec 18, 2024

Purpose

Render the new "Local File Path" annotation nicely. The goal is to display something like this:

File Path (Vast) /mypath/aics/software/apps/staging/fss/data/7fb/6fb/b0f/6bd/7d5/1db/a4c/1e4/2a6/2a5/c7/aicsfiles-test-a4cb9af6-bcfa-11ef-8fe8-0242ac110002.czi

Closes #368
Closes #369

Changes

  • I had to create some logic for renaming the annotation from "Local File Path." I ended up putting it in the HttpAnnotationService, which has a few benefits.
    • The implementation is simpler than putting the logic downstream (e.g., in the FileAnnotationList).
    • The renaming only applies to the FMS data source, so people running BFF from a CSV won't be surprised by it.

Testing

Notes

  • The name File Path (Local) was used before File Path (Vast). I don't have a strong preference: please weigh in. The new FMSAnnotationPresentationLayer makes it easy to change it up.
  • I'm not totally sure of the name FMSAnnotationPresentationLayer nor the choice to put this annotation-renaming logic here instead of FES or changing the name in LabKey and the ETL pipeline.

@lynwilhelm
Copy link
Copy Markdown

@pgarrison If we are specifically calling out VAST in the "Copy files to local NAS (VAST)" modal @BrianWhitneyAI was working on, perhaps we should be consistent and mention VAST here? Without potentially understanding the whole picture, I usually think you should be more general in terminology so that if VAST changes to something else someday, we won't have to worry about updates. BUT, I read Kevin M's recent comment somewhere that he was happy to see us calling VAST out specifically, which makes me think users have been confused in the past about what our local storage was called? To be consistent, maybe the label should say (Local VAST) or (Local/VAST)?

@pgarrison
Copy link
Copy Markdown
Contributor Author

pgarrison commented Dec 20, 2024

@pgarrison If we are specifically calling out VAST in the "Copy files to local NAS (VAST)" modal @BrianWhitneyAI was working on, perhaps we should be consistent and mention VAST here? Without potentially understanding the whole picture, I usually think you should be more general in terminology so that if VAST changes to something else someday, we won't have to worry about updates. BUT, I read Kevin M's recent comment somewhere that he was happy to see us calling VAST out specifically, which makes me think users have been confused in the past about what our local storage was called? To be consistent, maybe the label should say (Local VAST) or (Local/VAST)?

@lynwilhelm I realized I uploaded the wrong screenshot -- it's fixed now. The current version of this PR says "File Path (Vast)". Does that seem good to you, or do you think "File Path (Local Vast)" is more clear?

@pgarrison Classic;) IDK, I slightly lean toward calling it local storage too. Whatever you think...but last thing is VAST is an acronym so I think you should do uppercase, right?

Comment thread packages/core/components/FileDetails/FileAnnotationList.tsx Outdated
Comment thread packages/core/services/AnnotationService/HttpAnnotationService/index.ts Outdated
@pgarrison
Copy link
Copy Markdown
Contributor Author

pgarrison commented Dec 21, 2024

Lyndsay left the following comment.

@pgarrison Classic;) IDK, I slightly lean toward calling it local storage too. Whatever you think...but last thing is VAST is an acronym so I think you should do uppercase, right?

So I think this PR needs to be updated to "File Path (Local VAST)" before merging.

@BrianWhitneyAI

@BrianWhitneyAI BrianWhitneyAI merged commit ca2154b into feature/local_cloud_toggle Jan 2, 2025
@BrianWhitneyAI BrianWhitneyAI deleted the feature/368-use-local-file-path-annotation branch January 2, 2025 20:16
@pgarrison
Copy link
Copy Markdown
Contributor Author

Thanks @BrianWhitneyAI!

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.

6 participants