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

Out of range on sequence: show message on tooltip on click annotation #232

Open
wants to merge 23 commits into
base: feature/sync-sequence
Choose a base branch
from

Conversation

p3rcypj
Copy link

@p3rcypj p3rcypj commented Apr 18, 2024

Merge into feature/sync-sequence

📌 References

📝 Implementation

  • Features

    • Out of range annotation
    • Chunk when many Uniprot IDs
    • Created cache wrapper for all requests
  • Minor changes

    • Removed BLAST, was set almost true for all tracks (which I don't quite know if it should be like that)
    • Quick style modification: Gene viewer subtrack
    • Fix loading stop showing when was still loading

🎨 Screenshots

image
image

Screenshots showing: Out of range annotation

@p3rcypj p3rcypj changed the base branch from development to feature/sync-sequence December 31, 2024 03:41
@p3rcypj p3rcypj marked this pull request as ready for review December 31, 2024 12:35
@p3rcypj
Copy link
Author

p3rcypj commented Dec 31, 2024

Note for the cache feature: I used Session Storage to delete the data when the user closes the browser. Unfortunately, that makes also the data not accessible between other windows or tabs. But I think it is best to make the data be deleted when the tab is closed.

@p3rcypj p3rcypj requested a review from tokland January 2, 2025 09:20
Copy link

@tokland tokland left a comment

Choose a reason for hiding this comment

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

In-line comments:

@p3rcypj p3rcypj requested a review from tokland January 7, 2025 11:15
Copy link

@tokland tokland left a comment

Choose a reason for hiding this comment

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

In-line comments:

@p3rcypj p3rcypj requested a review from tokland January 19, 2025 04:08
Copy link

@tokland tokland left a comment

Choose a reason for hiding this comment

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

Some in-line comments:

const coverageDetails = {
type I18N = typeof i18n;

const getCoverageDetails = (i18n: I18N) => ({
Copy link

@tokland tokland Jan 20, 2025

Choose a reason for hiding this comment

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

No need to pass i18n as an argument. You can use the imported i18n normally (it's a state object, so changes will be there without further ado)

{ condition: notCovered, details: getCoverageDetails(i18n).notCovered },
{
condition: partiallyCovered && !covered,
details: getCoverageDetails(i18n).partiallyCovered,
Copy link

Choose a reason for hiding this comment

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

Used twice, so move to local variable: const coverageDetails = getCoverageDetails()

mappings: {
chain_id: ChainId;
struct_asym_id: ChainId;
}[];
Copy link

Choose a reason for hiding this comment

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

When we have complex types, like an object, we use Array<...>

const emdbMapping$ = getFromUrl<PdbEmdbMapping>(emdbMapping);
const chains = molecules
.flatMap(({ chains }) => chains)
.map(chain => ({
Copy link

Choose a reason for hiding this comment

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

We are building a ChainIdMapping, right? in that case, let's be explicit about it: (chain): ChainIdMapping. This not only helps TS giving better error message, it also link the structure to the type, so we get automatic renaming of props, code navigation, and so on.

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