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(agora): migrate Agora to use shared boxplot component (AG-1460) #2956

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hallieswan
Copy link
Collaborator

@hallieswan hallieswan commented Jan 8, 2025

Description

Migrates Agora boxplots to use shared boxplot component.

Related Issue

Changelog

  • Migrates Agora boxplots to use shared boxplot component
  • Removes historic Agora boxplot component dependent on dc.js (AG-1179)
  • Adds shared Storybook for Agora libraries with proof of concept stories for Boxplot, About, and News components

Preview

Boxplot:

AG-1460_boxplot.mov

Comparison:

Plot Current Updated
Proteomics AG-1460_boxplot_protein_tooltip_develop AG-1460_boxplot_protein_tooltip_monorepo
RNA AG-1460_boxplot_rna_develop AG-1460_boxplot_rna_monorepo

Run shared storybook with nx run agora-storybook:storybook.
View at http://localhost:4400/:

AG-1460_agora_storybook

Comment on lines -17 to -23
<agora-score-barchart
[shouldResize]="false"
[score]="20"
[barColor]="'#8B8AD1'"
[data]="scoreDistribution"
>
</agora-score-barchart>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not included in the original About page, so removed it here.

Comment on lines +34 to +41
"styles": [
"apps/agora/app/src/styles.scss",
"node_modules/primeicons/primeicons.css",
"node_modules/primeng/resources/primeng.min.css"
],
"stylePreprocessorOptions": {
"includePaths": ["libs/agora/styles/src/lib", "libs/agora/themes/src/lib"]
}
Copy link
Collaborator Author

@hallieswan hallieswan Jan 8, 2025

Choose a reason for hiding this comment

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

Boilerplate except for this section which imports Agora's styles, as described here

@@ -260,18 +261,21 @@ export class BoxplotChart {
max: yAxisMax ? yAxisMax + yAxisPadding : undefined,
},
tooltip: {
confine: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Prevents tooltips on individual points from overflowing the screen

Comment on lines +271 to +272
extraCssText:
'opacity: 0.9; width: auto; max-width: 300px; white-space: pre-wrap; text-align: center;',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

More closely match style of current point tooltips

@hallieswan hallieswan marked this pull request as ready for review January 8, 2025 23:08
@hallieswan hallieswan requested review from tschaffter and a team as code owners January 8, 2025 23:08
@hallieswan hallieswan self-assigned this Jan 8, 2025
Copy link
Member

@tschaffter tschaffter left a comment

Choose a reason for hiding this comment

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

LGTM! The PR description, along with the preview one, is top-notch. I've shared a couple of comments.

.gitignore Outdated Show resolved Hide resolved
@@ -0,0 +1,24 @@
import { boxPlotChartItem } from '@sagebionetworks/agora/models';

export const boxPlotChartItemsMock: boxPlotChartItem[] = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a similar mock in agora-testing, but I ran into issues importing that library when running Storybook. For simplicity, I created the mock here. However, long term, we may want to consider either pulling this data from the agora-testing library or creating a separate agora-mocks library.

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.

2 participants