-
-
Notifications
You must be signed in to change notification settings - Fork 23
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: Frontend in React #31
Conversation
@alexgarel @stephanegigandet I'll be working on this PR for the frontend. |
<Route path="/entry/:id" element={<EditEntry />} /> | ||
</Routes> | ||
</div> | ||
</Router> |
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 don't know if it can be moved easily later on (and in this case we can let it like that for now), but I think in the long run, we will have something like: "/:branch-name/:taxonomy/entry/:id"
- branch-name would be the name of a branch in git
- taxonomy would be taxonomy name (like categories)
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.
@alexgarel Yep, I think it would be possible. The "branch-name" property might be a little difficult to bring in right away, due to its complexity related to auth and error handling. The "taxonomy" property can be brought in, whenever the provision of seperate databases for each taxonomy is present in Neo4J.
@@ -0,0 +1,36 @@ | |||
import { useState, useEffect } from "react"; | |||
|
|||
const useFetch = (url) => { |
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.
NOT relevant for this PR, but what about just using a library for all the fetching and stuff: query does an excelent job on that and it is simple to use
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 think this kind of suggestion is useful @nobeeakon
introducing right libraries up front to avoid reinventing the wheel latter on ;-)
(but this is a difficult equilibrium to get unless you are experimented, so cool to see suggestions on that).
taxonomy-editor-frontend/src/pages/editentry/ListAllOtherProperties.jsx
Outdated
Show resolved
Hide resolved
taxonomy-editor-frontend/src/pages/editentry/ListAllOtherProperties.jsx
Outdated
Show resolved
Hide resolved
taxonomy-editor-frontend/src/pages/editentry/ListAllOtherProperties.jsx
Outdated
Show resolved
Hide resolved
taxonomy-editor-frontend/src/pages/editentry/ListAllOtherProperties.jsx
Outdated
Show resolved
Hide resolved
taxonomy-editor-frontend/src/pages/editentry/ListAllProperties.jsx
Outdated
Show resolved
Hide resolved
taxonomy-editor-frontend/src/pages/editentry/AccumulateAllComponents.jsx
Outdated
Show resolved
Hide resolved
taxonomy-editor-frontend/src/pages/editentry/FetchRelations.jsx
Outdated
Show resolved
Hide resolved
|
||
const ListAllProperties = ({ nodeObject, setNodeObject }) => { | ||
let toBeRendered = [] | ||
|
||
function guidGenerator() { |
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.
don't understand what's the value of doing this?
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.
It's to generate a unique key for each row of the properties table.
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.
thanks, I see, mmm, if you need this, possibly the place to do it is when loading the data or inside a useEffect
as proposed below, that way you know when it will run and why it will run. Also you would need to do it on every action (delete, add new item)...
(think this may help)
editable={{ | ||
onRowAdd: (newRow) => new Promise((resolve, reject) => { | ||
const updatedRows = [...data, { id: Math.floor(Math.random() * 100), ...newRow }] | ||
setTimeout(() => { |
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.
mm, why the timeout?
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.
Using a timeout for allowing it to update the state.
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 may be missing something, but as far as I can see, it is not needed
resolve() | ||
}, 2000) | ||
}), | ||
onRowDelete: selectedRow => new Promise((resolve, reject) => { |
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.
mmm, from my understanding this may be better to keep it in a different PR as this one is already super big. Also, do you want to store those changes in the database?, if not I don't see a lot of value in having a delete update, 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.
@nobeeakon Yep, all the changes need to be tracked, so that I'll be able to update it in the database.
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.
still, please if possible, do not continue adding features in 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.
amm, you may want to do the databse update and the client update in a single PR so you have the whole feature, as that may affect the implementation
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.
Yes @nobeeakon, after successful implementation of CRUD operations in the table present in ListAllProperties.jsx
, we can merge after your approval. You have also been invited to the taxonomy-editor team. Hence after accepting it, you would be able to merge PRs too.
'property-name': property_name, | ||
'property-value': nodeObject[key] | ||
}) | ||
} | ||
}); | ||
|
||
const [data, setData] = useState(toBeRendered); |
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.
better not call it this way, please prefer to have the hooks before anything else is being called, as they can't be after conditionals, etc. You could instead declare it in line 7
and set it later, possibly in a useEffect
WARNING: this PR is being dispatched in more than one PR.
What
Related issue(s)