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): agora fixes (AG-1604) and Q4 2024 changes (AG-1605) #2955

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

Conversation

sagely1
Copy link
Contributor

@sagely1 sagely1 commented Jan 4, 2025

Description

This is a collection of fixes found during manual testing (AG-1604)
This also incorporates changes from Q4 2024 (AG-1605)

Fixes

  • Fixed GCT loading animation was not displaying when loading the page. New CSS was written to support the new standalone component.
  • Download chart functionality was not working with FireFox and the latest download-dom-image-more package. It is a known issue so the width and height are now being explicitly passed to maintain cross-compatibility.
  • Due to the removal of Bootstrap, the Resources tab in Gene Details was not displaying the resource cards properly so new CSS was written to achieve a similar effect. The original code has element inconsistently aligned either left or center so this has been updated to left justified.
  • SHA in footer was displaying the full SHA so this was updated to show the shortened SHA.

@sagely1 sagely1 changed the title fix loading animation for gct, cards in details page, shortened sha feat(agora): agora fixes (AG-1604) Jan 4, 2025
@sagely1 sagely1 changed the title feat(agora): agora fixes (AG-1604) feat(agora): agora fixes (AG-1604) and Q4 2024 changes (AG-1605) Jan 4, 2025
@@ -7,6 +7,6 @@ DB_HOST="agora-mongo" # must match mongo service name

# specifies data release manifest and team images folder
DATA_FILE="syn13363290"
DATA_VERSION="68"
DATA_VERSION="71"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updating data_version to match current Agora production version

Comment on lines -12 to -13
<div class="row tab-row last-row">
<div id="target-enabling-resources-card1" class="col-md-3 col-sm-8 col-8 header-title">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

col-sm* and col-md* are part of the bootstrap dependency that was removed for migration to the monorepo

@sagely1 sagely1 marked this pull request as ready for review January 6, 2025 17:04
@sagely1 sagely1 requested review from a team and tschaffter as code owners January 6, 2025 17:04
Comment on lines +1 to +5
<pre>
isActive: {{ isActive }}
isGlobal: {{ isGlobal }}
</pre>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be included here?

}

ngOnInit(): void {
this.dataVersion$ = this.dataVersionService.getDataversion();
this.sha$ = this.gitHubService.getCommitSHA('agora/v0.0.2');
this.sha$ = this.gitHubService.getCommitSHA(this.tag);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about creating a ticket to use appVersion and adding a related TODO here? Just thinking it would be great to capture your thoughts from our chat earlier today about tracking the app version in one place once the tag format is finalized!

Comment on lines +21 to +25
console.log('global', this.isGlobal);
if (this.isGlobal) {
this.helperService.loadingChange.subscribe(() => {
this.isActive = this.helperService.getLoading();
console.log('active', this.isActive);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the log statements be removed?

Suggested change
console.log('global', this.isGlobal);
if (this.isGlobal) {
this.helperService.loadingChange.subscribe(() => {
this.isActive = this.helperService.getLoading();
console.log('active', this.isActive);
if (this.isGlobal) {
this.helperService.loadingChange.subscribe(() => {
this.isActive = this.helperService.getLoading();

@@ -8,5 +8,5 @@
"types": []
},
"exclude": ["src/test-setup.ts", "**/*.spec.ts", "**/*.test.ts", "jest.config.ts"],
"include": ["**/*.ts"]
"include": ["**/*.ts", "src/lib/components/gene-similar/gene-similar.component.spec.ts"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, why does gene-similar need to be included here, but the other subcomponents don't need to be included (e.g. gene-table)?

"**/*.spec.ts",
"**/*.d.ts",
"jest.config.ts",
"src/lib/components/gene-similar/gene-similar.component.spec.ts"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would the gene-similar test already be picked up by "**/*.spec.ts", so this line may not be needed?

<div class="gene-hero-aliases">
<h4 class="gene-hero-aliases-heading">Also known as</h4>
@if (getEnsemblUrl() !== '') {
<p>
@if (getEnsemblUrl()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@if (getEnsemblUrl()) {
@if (getEnsemblUrl(); as ensembleUrl) {

Copy link
Member

Choose a reason for hiding this comment

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

Reference the result of the function and use it in the if block, instead of calling the getter twice.

// Authorization: `Bearer ${this.token}`,
// });

// return this.http.get<any[]>(this.apiUrl, { headers }).pipe(
return this.http.get<any[]>(this.apiUrl).pipe(
Copy link
Member

@tschaffter tschaffter Jan 6, 2025

Choose a reason for hiding this comment

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

Specify the type of the output instead of using any[]. Either using an interface or this.http.get<Array<{ name: string; commit: { sha: string } }>>.

@@ -25,6 +25,7 @@
"libs/agora/gene-comparison-tool/src/index.ts"
],
"@sagebionetworks/agora/gene-details": ["libs/agora/genes/src/index.ts"],
"@sagebionetworks/agora/gene-similar": ["libs/agora/genes/src/index.ts"],
Copy link
Member

Choose a reason for hiding this comment

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

This line may be referencing the incorrect index file (currently the same as @sagebionetworks/agora/gene-details).

Copy link
Member

Choose a reason for hiding this comment

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

Same comment actually for @sagebionetworks/agora/gene-details, which references the same index file as @sagebionetworks/agora/genes.

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.

3 participants