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

MSA viz using Nightingale #32

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

Conversation

triksi254
Copy link

Step 1: Install Dependencies
npm i @nightingale-elements/nightingale-msa

Step 2: Create a Wrapper Component
New file NightingaleMSAWrapper.js

Step 3: Integrate the Wrapper Component
Modify your Outputs component to include the NightingaleMSAWrapper

Step 4: Parse the MSA Data in NewAnalysis.tsx

Step 5: Integrate the Parsed Data into the Outputs Component

Copy link

netlify bot commented Oct 12, 2024

Deploy Preview for molevolvr failed.

Name Link
🔨 Latest commit 32e08e9
🔍 Latest deploy log https://app.netlify.com/sites/molevolvr/deploys/671d5d76b38dbb00098ae0e2

@triksi254
Copy link
Author

@vincerubinetti @jananiravi please review and give feedback, thank you

@triksi254 triksi254 closed this Oct 12, 2024
@triksi254 triksi254 deleted the MSA_viz branch October 12, 2024 18:24
@triksi254 triksi254 restored the MSA_viz branch October 12, 2024 18:24
@triksi254 triksi254 reopened this Oct 12, 2024
@vincerubinetti
Copy link
Collaborator

A couple of things just briefly looking at the code and not testing it yet:

  • We're using Bun for package management, so package-lock.json should not be there, and you should install things with bun install instead of npm install
  • The code needs to be indented properly, which can be done automatically by running bun run lint in the /frontend directory.
  • Stylistically, we're using type instead of interface for type declarations.
  • React.FC<NightingaleMSAWrapperProps> type is fine, but to be consistent with the rest of the code base, it should really just be named Props. Then you'd do { sequences, features }: Props. See other components for examples.

It's perfectly okay that you've set it up the way you did, because I didn't provide any guidelines in the issue, but I'll want to make a few architectural changes:

  • Move low-level, Nightingale-specific options to within the component, e.g. mouseOverFillColor. We want components to have some options for customization to fit our needs, but mostly they should be simple to use from the outside and handle things automatically.
  • For now, instead of adding an example use case to the Outputs.tsx section, let's add it to the Testbed.tsx page with completely fake, generated data, like we're doing with the network visualization component.
  • It looks like you're adding some things to the NewAnalysis.tsx page which takes whatever the user has inputted and pass it to the MSA component. I'm not sure if we're doing much visualization of raw inputs... that is, I think the data that will ultimately be passed to the MSA component will be coming from the backend, from analysis results, not something that the user inputted verbatim. However I could be very wrong on this. Even if I am wrong though, I will want to set up a generic way that passes all the raw inputs through to the results page, so that any visualizations we have there can use any of the raw inputs. As such, we don't need the changes to NewAnalysis.tsx yet.

I'll probably have some more comments on Monday.

@vincerubinetti
Copy link
Collaborator

@triksi254 If you're able to make the changes I discussed above, please do. If not, I won't be able to get to making the changes myself until next week.

@triksi254
Copy link
Author

triksi254 commented Oct 15, 2024 via email

@triksi254
Copy link
Author

bun run lint test shows the error below
bun: File name too long:
error: script "lint" exited with code 1

@vincerubinetti
Copy link
Collaborator

I'm not able to reproduce that error. Can you provide more info, e.g. which commands you ran, what OS you're using, what other logs (errors or otherwise) did you get?

@triksi254
Copy link
Author

triksi254 commented Oct 16, 2024

$ eslint . --fix && prettier **/*.{tsx,ts,module.css,css,html,md,json,yaml} --write --no-error-on-unmatched-pattern

=============

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=4.7.4 <5.6.0

YOUR TYPESCRIPT VERSION: 5.6.2

Please only submit bug reports when using the officially supported version.

=============
bun: File name too long: 
error: script "lint" exited with code 1` 
This is the full error. Am using windows 10-64 bit

@vincerubinetti
Copy link
Collaborator

Also, this typescript is defining a custom element for nightingale...

declare namespace JSX {
  type IntrinsicElements = {
    "nightingale-msa": any;
  };
}

...but it is overwriting/removing the definitions for all other native HTML elements:

Screenshot 2024-10-16 at 11 15 21 AM

I believe to capture both it should be:

namespace JSX {
  interface IntrinsicElements {
    "nightingale-msa": JSX.HTMLAttributes<CustomElement>;
  }
}

@triksi254
Copy link
Author

triksi254 commented Oct 16, 2024 via email

@vincerubinetti
Copy link
Collaborator

See what I did in this PR for more guidance:

#39

Copy link
Author

@triksi254 triksi254 left a comment

Choose a reason for hiding this comment

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

Reviewed the IPR changes and incorporated some of the revisions mentioned earlier @vincerubinetti

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