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

feat: update annotation row name #1163

Merged
merged 5 commits into from
Oct 2, 2024
Merged

feat: update annotation row name #1163

merged 5 commits into from
Oct 2, 2024

Conversation

codemonkey800
Copy link
Contributor

@codemonkey800 codemonkey800 commented Sep 19, 2024 •
edited
Loading

#1050

  • Updates the annotation row name to be in the format {file_id} - {object_name}
  • Add annotation ID + move ground truth badge
  • Adds new annotation name to download modal

Demo

https://dev-annotation-row-name.cryoet.dev.si.czi.technology/

image image image

@codemonkey800
Copy link
Contributor Author

@Janeece @kev-zunshiwang could you review the design please 🙏 I'll have a demo link up soon. one thing I wasn't able to do was add the extra table space because it's not possible to add padding in table rows:

image

@codemonkey800 codemonkey800 changed the title update annotation row name feat: update annotation row name Sep 19, 2024
bchu1
bchu1 reviewed Sep 19, 2024
View reviewed changes
frontend/packages/data-portal/app/components/Download/ConfigureDownloadContent.tsx
@@ -9,12 +9,16 @@ import { ConfigureTomogramDownloadContent } from './ConfigureTomogramDownloadCon
export function ConfigureDownloadContent() {
const { t } = useI18n()
const { datasetTitle, runName, objectName } = useDownloadModalContext()
const { annotationId, objectShapeType } = useDownloadModalQueryParamState()
const { annotationName, annotationId, objectShapeType } =
Copy link
Contributor

Choose a reason for hiding this comment

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

could we just derive the name from DownloadModalContextValue.annotationToDownload.id and DownloadModalContextValue.annotationToDownload.annotation.object_name?

also, bit confused cuz I didn't see setAnnotationName() called anywhere 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

oh it's calling openAnnotationDownloadModal() 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I think that would be more preferred, I want to eventually refactor the download modal state and that includes getting the full annotation data from the backend using the ID 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

don't we already have annotationToDownload though? (recently renamed from activeAnnotation 😬)

@Janeece
Copy link
Collaborator

Janeece commented Sep 20, 2024 •
edited
Loading

@codemonkey800 (based on the screenshots)

  • No problem about not adding the extra space to the row padding, I think its better we don't customize that anyway (cc @kev-zunshiwang, sorry if i missed that in design reviews).
  • For the annotation name table cell, can we increase the distance between the secondary text (ID+ tag) and the tertiary text (authors) to space-s? It seems like all text rows are are equal distances from each other, but the tertiary text should have a larger gap above it. Edit: it should match the spacing like we see on the all datasets table

@Janeece
Copy link
Collaborator

Janeece commented Sep 20, 2024

@codemonkey800 double checked in the dev environment & i have one more comment -- the Authors text (tertiary text) seems darker than on the all datasets table -- can you double check that it is the right color - text-secondary?
Screenshot 2024-09-20 at 2 22 58 PM
Screenshot 2024-09-20 at 3 26 12 PM

@codemonkey800
Copy link
Contributor Author

@codemonkey800 double checked in the dev environment & i have one more comment -- the Authors text (tertiary text) seems darker than on the all datasets table -- can you double check that it is the right color - text-secondary? Screenshot 2024-09-20 at 2 22 58 PM Screenshot 2024-09-20 at 3 26 12 PM

oops yeah was using the wrong color, it should be fixed now!

@Janeece
Copy link
Collaborator

Janeece commented Oct 1, 2024

@codemonkey800 looks great, thank you!

@codemonkey800 codemonkey800 merged commit 6164201 into main Oct 2, 2024
14 checks passed
@codemonkey800 codemonkey800 deleted the dev-annotation-row-name branch October 2, 2024 18:16
@github-actions github-actions bot mentioned this pull request Oct 1, 2024
Merged
github-actions bot added a commit that referenced this pull request Oct 3, 2024
@github-actions
chore(main): release web 1.31.0 (#1177)
5c839dd 
🤖 I have created a release *beep* *boop*
---


##
[1.31.0](web-v1.30.0...web-v1.31.0)
(2024-10-03)


### ✨ Features

* Add portal standard badge to tomograms table and integrate alignment
metadata accordion with new API
([#1179](#1179))
([1de70a1](1de70a1))
* Add standard tomogram badge to tomogram selector
([#1181](#1181))
([ac4ad27](ac4ad27))
* Add tooltip to Alignment ID field in tomogram sidebar
([#1196](#1196))
([fad5207](fad5207))
* Integrate more V2 API queries
([#1174](#1174))
([92f80a7](92f80a7))
* Migrate and start using tomograms query in UI
([#1169](#1169))
([d21ffd9](d21ffd9))
* Undo changes for transforming annotations and add new callout for
mismatched alignments
([#1191](#1191))
([ea1598d](ea1598d))
* update annotation row name
([#1163](#1163))
([6164201](6164201))
* Update to generic annotation transformation callout during download
([#1186](#1186))
([5597524](5597524))


### 🐞 Bug Fixes

* address misc UI/UX bugs
([#1192](#1192))
([178737d](178737d))
* change incorrect deposition links
([#1133](#1133))
([dc642e5](dc642e5))
* Stop querying depositionTitle
([#1187](#1187))
([07a08d4](07a08d4))


### 🧹 Miscellaneous Chores

* prefix IDs across the portal
([#1178](#1178))
([1732fb2](1732fb2))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kne42 kne42 kne42 approved these changes

@ehoops-cz ehoops-cz Awaiting requested review from ehoops-cz

@bchu1 bchu1 Awaiting requested review from bchu1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@codemonkey800 @Janeece @kne42 @bchu1