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

feat: cell type info and info panel refactor #1043

Merged
merged 29 commits into from
Jul 23, 2024
Merged

Conversation

kaloster
Copy link
Contributor

@kaloster kaloster commented Jul 18, 2024

Adding Cell Type to the info panel #1050

rdev -> here

  • Added endpoint to fetch cell type info from s3 assets
  • Added Cell Type tab to info panel
  • Added info icon to cell types to trigger cell type tab
  • Refactored both Gene and Cell Type component to use common components and refactored code
  • Fixed existing tests
  • Update analytics event
  • Update loader to SDS
  • Added e2e
  • Added unit tests
  • Create a ticket for Part 3 - search functionality for both Gene and Cell Type
  • Create a ticket for - Adjust info icon position - this has a cascading effect on the layout - extracting this to a separate ticket to remain in scope

@kaloster kaloster added the stack label Jul 19, 2024
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 86.79245% with 7 lines in your changes missing coverage. Please review.

Project coverage is 77.57%. Comparing base (97f57e6) to head (dc5d162).
Report is 13 commits behind head on main.

Files Patch % Lines
server/common/rest.py 70.83% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1043      +/-   ##
==========================================
+ Coverage   77.43%   77.57%   +0.13%     
==========================================
  Files          90       92       +2     
  Lines        7010     7116     +106     
==========================================
+ Hits         5428     5520      +92     
- Misses       1582     1596      +14     
Flag Coverage Δ
frontend 77.57% <86.79%> (+0.13%) ⬆️
javascript 77.57% <86.79%> (+0.13%) ⬆️
unitTest 77.57% <86.79%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kaloster kaloster force-pushed the kaloster/cell-type-info branch from 39d7cd2 to 0245fd4 Compare July 19, 2024 20:36
@kaloster kaloster force-pushed the kaloster/cell-type-info branch from 61b5dad to ddc0fec Compare July 19, 2024 21:01
@kaloster kaloster force-pushed the kaloster/cell-type-info branch 2 times, most recently from 7b794ab to 68af505 Compare July 20, 2024 00:15
@kaloster kaloster force-pushed the kaloster/cell-type-info branch from a61305a to 50dae4e Compare July 20, 2024 00:29
@@ -1382,6 +1385,9 @@ for (const testDataset of testDatasets) {
test("open info panel and hide/remove", async ({
page,
}, testInfo) => {
skipIfSidePanel(graphTestId, MAIN_PANEL);
skipIfPbmcDataset(testDataset, DATASET);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skipping for pbmc3k.cxg dataset as there is no cell_type category

Copy link
Contributor

Choose a reason for hiding this comment

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

Great context! Can we add that as code comment please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I added an item in the Q3 list to consolidate e2e test dataset to just hopefully one, so we don't have to deal with so many different test datasets 😆

} from "../infoPanelParts";
import { ExtendedInfoProps } from "../types";

function ContainerInfo(props: ExtendedInfoProps) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Common container for either Gene Info or Cell Type Info

Copy link
Contributor

Choose a reason for hiding this comment

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

Great extraction 👏

@kaloster kaloster force-pushed the kaloster/cell-type-info branch from 023298f to dc6815e Compare July 22, 2024 22:12
@kaloster kaloster marked this pull request as ready for review July 22, 2024 22:18
@tihuan
Copy link
Contributor

tihuan commented Jul 23, 2024

Quick feedback on styling, the cell type row seems to be taller compared to the author category items now, maybe we can lower the min-height/width from 24 to 18px or something?

BEFORE (24px)
Screenshot 2024-07-23 at 8 24 05 AM

AFTER (18px)
Screenshot 2024-07-23 at 8 25 46 AM

Video:

Screen.Recording.2024-07-23.at.8.27.28.AM.mov

CC: @hthomas-czi to see what you think!

Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

Overall looks super good! Thanks so much, @kaloster 🚀 Just some small nits otherwise LGTM!

@@ -1382,6 +1385,9 @@ for (const testDataset of testDatasets) {
test("open info panel and hide/remove", async ({
page,
}, testInfo) => {
skipIfSidePanel(graphTestId, MAIN_PANEL);
skipIfPbmcDataset(testDataset, DATASET);
Copy link
Contributor

Choose a reason for hiding this comment

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

Great context! Can we add that as code comment please?

@@ -143,3 +143,30 @@ export interface PublisherMetadata {
published_month: number;
published_year: number;
}

export interface CellInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome types man!

client/src/components/geneExpression/gene.tsx Show resolved Hide resolved
} from "../infoPanelParts";
import { ExtendedInfoProps } from "../types";

function ContainerInfo(props: ExtendedInfoProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great extraction 👏

client/src/components/geneExpression/infoPanel/connect.ts Outdated Show resolved Hide resolved
@kaloster kaloster requested a review from tihuan July 23, 2024 18:52
@kaloster kaloster requested a review from tihuan July 23, 2024 21:44
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

One tiny comment then LGTM!! Sorry 😆

client/src/components/geneExpression/infoPanel/connect.ts Outdated Show resolved Hide resolved
@kaloster kaloster force-pushed the kaloster/cell-type-info branch from dc5d162 to bf4bef9 Compare July 23, 2024 22:15
@kaloster kaloster enabled auto-merge (squash) July 23, 2024 22:16
@kaloster kaloster merged commit f4ddb1a into main Jul 23, 2024
18 of 19 checks passed
@kaloster kaloster deleted the kaloster/cell-type-info branch July 23, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants