-
Notifications
You must be signed in to change notification settings - Fork 2
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
Display all relations #54
Conversation
Also in preparation for you combine repo @pkalita-lbl we can discuss about some questionable dependences here. We can find better alternatives or inhouse. I have added moment, but it seems it is not favored. If you have an alternative And also dbxref idk what it is doing it has like one function etc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My immediate concern is about the use of api-url
in src/styled.html
.
Everything else looks fine... I think?? Am I right in understanding that a lot of this code is copy and pasted (and maybe lightly modified?) from here https://github.com/geneontology/noctua-form-base/tree/master/src/%40noctua.form?
If that's the case we need to figure out a better way to make the shared code accessible to whatever clients need it. To be clear I'm not saying to do that as part of this PR, and maybe it's even a separate project. But the current situation seems somewhat challenging to maintain.
@@ -31,6 +31,7 @@ | |||
"cytoscape-dagre": "^2.4.0", | |||
"graphlib": "^2.1.8", | |||
"lodash": "^4.17.21", | |||
"moment": "^2.30.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably already know that Moment has their own recommendations for alternatives: https://momentjs.com/docs/#/-project-status/recommendations/
Personally I've used date-fns and have had no issues with it. But I think that's something that can be addressed separately from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am having problems with date-fns and typescript, 'Intl' has no exported member named 'RelativeTimeFormatOptions'. Did you mean 'DateTimeFormatOptions'? so can I look into this later then I remove moment later. or moment has to be removed before PR. I had changed the date to use date-fns and working but that tpescript error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without knowing what you tried doing, the error message in isolation isn't too informative. I'm not even really strongly advocating for date-fns
. I was just pointing out that I've used it (yes, in TypeScript projects) and thought it was fine.
But more importantly, to quote myself 😂 :
I think that's something that can be addressed separately from this PR.
@pkalita-lbl Yes, most of the code is from noctua form base VPE and I am planning to fix that if they agree to continue this route. It seemed there was a new proposal of using NDEx or some other lib. So I didn't want to spend time trying to make it accessible if it is temporary solution. However, I once did have the npm and then https://www.npmjs.com/package/@geneontology/noctua-form-base But refactoring more, maybe only few functions are needed because this is a readonly and noctua-form-base is for the whole CRUD |
That's long, long, long-term thinking. I wouldn't even call it a proposal or a plan. It's a glimmer of an idea that may or may not happen. If it does happen it won't be for a while.
I think that's more along the lines of what I was imagining: moving the shared pieces to a separate NPM package that can be used by the VPE and the widget. Maybe some day we can chat with @kltm about how much work that would be and how to prioritize it among other work. |
Issues
Changed
In Progresss
@pkalita-lbl are you using show-popup property, I kinda have deleted it. I am not sure if its is unused code
cc @vanaukenk @thomaspd @dustine32