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

Add variant recommendation page #426

Open
wants to merge 3 commits into
base: feat/var-rec
Choose a base branch
from

Conversation

gebbing12
Copy link

Code for the Variant Recommendation Page - frontend and backend implementation

pom.xml Outdated
@@ -321,6 +321,11 @@
<artifactId>firebase-admin</artifactId>
<version>9.1.1</version>
</dependency>
<dependency>
<groupId>com.googlecode.json-simple</groupId>

Choose a reason for hiding this comment

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

Do we need to add this? Will GSON work?

@@ -22,5 +22,7 @@ public final class Constants {

public static final String DEFAULT_GENE_SYNONMN_SOURCE = "cBioPortal";

public static final String ONCOKB_S3_BUCKET = "oncokb-bucket";

Choose a reason for hiding this comment

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

@zhx828 do you want this to be an environment variable?

String line;
while ((line = reader.readLine()) != null) {
String[] data = line.split("\t");
JSONObject jsonObject = new JSONObject();

Choose a reason for hiding this comment

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

Are you able to use Gson?

Comment on lines 93 to 107
input[type='range']::-webkit-slider-thumb {
pointer-events: all;
width: var(--_thumb-size);
height: var(--_thumb-size);
border-radius: 0;
border: 0 none;
}

input[type='range']::-moz-range-thumb {
pointer-events: all;
width: var(--_thumb-size);
height: var(--_thumb-size);
border-radius: 0;
border: 0 none;
}

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Author

Choose a reason for hiding this comment

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

This is to define the appearance of the slider thumb. -webkit-slider-thumb is for Chrome and Safari, while -moz-range-thumb is for Firefox.

Choose a reason for hiding this comment

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

According to mdn it looks like safari doesn't support this yet. (it's also in pre-release so that doesn't mean safari never will)
Does it still look good in safari?

Comment on lines 4 to 7
formatted = formatted.replace(/\.?0+$/, '');
if (formatted.endsWith('.')) {
formatted = formatted.slice(0, -1);
}

Choose a reason for hiding this comment

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

Is this to needed to remove trailing zeros? let formatted = +(value * 1000).toFixed(2); should give you the same result.

Copy link
Author

Choose a reason for hiding this comment

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

I'd like to display 100% instead of 100.00%, as it looks better. However, if it doesn't matter, I can remove it.

Choose a reason for hiding this comment

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

It makes me nervous to use a regex since it's difficult to see if they will work the way you want them to. Does this line of code remove the extra zeros? let formatted = +(value * 1000).toFixed(2);.

onChange: (newRange: [number, number]) => void;
}

export const TwoRangeSlider = ({ min, max, range, step, onChange }: RangeSliderProps) => {

Choose a reason for hiding this comment

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

Why is this called TwoRangeSlide, why not just Slider?

Comment on lines +96 to +97
if (!filter.value) return true;
return String(row[filter.id]).toLowerCase().includes(filter.value.toLowerCase());

Choose a reason for hiding this comment

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

See if you can turn this into a function. I see the same repeated several times.

Comment on lines +62 to +64
Filter({ filter, onChange }) {
return <Input placeholder="Search" value={filter ? filter.value : ''} onChange={event => onChange(event.target.value)} />;
},

Choose a reason for hiding this comment

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

Could this be made into a component? I see this code repeated in a few places.

Comment on lines 95 to 103
Filter({ filter, onChange }) {
const filterValue = filter ? filter.value : ['', ''];
return (
<div style={{ display: 'flex', gap: '10px' }}>
<Input placeholder="Min" value={filterValue[0]} onChange={event => onChange([event.target.value, filterValue[1]])} />
<Input placeholder="Max" value={filterValue[1]} onChange={event => onChange([filterValue[0], event.target.value])} />
</div>
);
},

Choose a reason for hiding this comment

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

Can this be made into a share component?

@@ -44,6 +45,7 @@ const Routes: React.FunctionComponent<IRoutesProps> = (props: IRoutesProps) => {
<PrivateRoute exact path={PAGE_ROUTE.SEARCH} component={SearchPage} />
<PrivateRoute path={PAGE_ROUTE.CURATION} component={CurationRoutes} hasAnyAuthorities={[AUTHORITIES.CURATOR]} />
<PrivateRoute exact path={PAGE_ROUTE.ACCOUNT} component={Account} hasAnyAuthorities={[AUTHORITIES.USER]} />
<PrivateRoute exact path={PAGE_ROUTE.VARIANR_REC} component={VariantRecListPage} />

Choose a reason for hiding this comment

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

Let's add hasAnyAuthorities={[AUTHORITIES.CURATOR]}.

Could you also add authorization to the backend too?

@gebbing12 gebbing12 requested a review from jfkonecn August 29, 2024 16:30
}

return ResponseEntity.ok(jsonList);
return ResponseEntity.ok(jsonArray.toString());
Copy link

Choose a reason for hiding this comment

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

Could you double that the content type is Content-Type: application/json; in the response header?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@gebbing12 gebbing12 requested a review from jfkonecn September 3, 2024 16:49
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