Create Single Corpus Component #46
Conversation
…rs to use a modal to update documents in the corpus
| .then(response => response.json()) | ||
| .then(data => { | ||
| setCorpusData(data); | ||
| fetch("/api/all_documents") |
There was a problem hiding this comment.
I'm guessing this is a temporary measure to help you all progress through this part of the UI, but I'd recommend we create an endpoint for retrieving only a subset of all_documents based on the Corpus.documents list and returning whatever useful information we need to display here (probably the same set of things we display on the documents page). If we imagine a future where we have thousands of Document objects in the database, this would require anyone who accesses any Corpus to retrieve some data representation of all of those and then filter them on the front end. That'll never be as efficient as letting the database query do that sorting.
There was a problem hiding this comment.
We currently call /api/all_documents because we wanted to display all the documents in the Modal to allow users to update which documents they want to include in the corpus. Should we only call /api/all_documents when we want to update the documents in the corpus or should we look into another way of allowing users to update the documents in the corpus?
There was a problem hiding this comment.
Ah, thanks for the clarification. That makes total sense. In the future, we'd likely want some kind of searching mechanism (i.e., find document by title or something similar). For now, retrieving a simplified view of all documents makes sense for the use case.
| docsData.map((doc) => setContainsDoc((values) => ({ | ||
| ...values, | ||
| [doc.id]: data.documents.includes(doc.id) | ||
| }))); |
There was a problem hiding this comment.
Pedantically, we probably want to .forEach here rather than .map, since we're not transforming docsData for any later purpose.
| .then(response => response.json()) | ||
| .then(data => { | ||
| setCorpusData(data); | ||
| }); |
There was a problem hiding this comment.
At a quick glance, I'd assume we could use some kind of loading state here.
frontend/components/Corpus.js
Outdated
| const docsList = () => { | ||
| return ( | ||
| <> | ||
| <h5>Documents:</h5> |
There was a problem hiding this comment.
I think this header is the wrong level given the rest of the document. I think the only higher header levels above this in the DOM tree are <h1> and, presumably, an <h2> to replace the exisiting <h5> below, so this should probably be an <h3>. We can always restyle it if we don't want it to default to the <h3> styling.
frontend/components/Corpus.js
Outdated
| `(${doc.year})`} | ||
| </li> | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm assuming that this map is working totally fine, but it looks like we're not supplying an explicit return value in the hypothetical else case, which I assume means this array is defaulting to supplying undefined in place of the non-targeted documents. This is working, but just to be more explicit, I think we could .filter out the allDocData and then map. That is: use filter to create the data structure we want and then map to convert that data structure to the DOM elements. That lets each array method handle its sole purpose rather than having map implicitly handling both. We could probably even pull up a new const that performs that filter and use it here and elsewhere.
frontend/components/Corpus.js
Outdated
| ? <p>Currently loading Corpus...</p> | ||
| : <div> | ||
| <h1>Corpus: {corpusData.title}</h1> | ||
| <h5>Description</h5> |
There was a problem hiding this comment.
As above, we should use an appropriate header level here (<h2>).
There was a problem hiding this comment.
This looks great! You've written some very elegant React, so fantastic job. I've left a few comments below for some cleanup items that should be quick to knock out (or, if you disagree, please let me know). I'll leave my approval here for when you've addressed them and are ready to merge this in. Thanks!
commit 427e51e Merge: a267410 590b56e Author: Peihua Huang <[email protected]> Date: Wed Jul 21 19:35:28 2021 -0400 Merge pull request #46 from dhmit/create_single_corpus_component Create Single Corpus Component commit 590b56e Author: Peihua Huang <[email protected]> Date: Wed Jul 21 19:35:18 2021 -0400 updated map function such that the data is first filtered then mapped commit 2d88e42 Author: Peihua Huang <[email protected]> Date: Wed Jul 21 19:32:13 2021 -0400 updated heading tags and loading states commit 584c092 Merge: ddfa24d ead126f Author: Peihua Huang <[email protected]> Date: Mon Jul 19 17:31:48 2021 -0400 Merge branch 'main' into create_single_corpus_component commit ddfa24d Merge: f2fd0e1 e0d24bc Author: Peihua Huang <[email protected]> Date: Mon Jul 19 17:22:06 2021 -0400 Merge branch 'main' into create_single_corpus_component commit f2fd0e1 Author: Peihua Huang <[email protected]> Date: Fri Jul 16 17:03:50 2021 -0400 italicized the title of documents and fixed linter warnings commit 9e1e249 Author: Peihua Huang <[email protected]> Date: Fri Jul 16 16:58:01 2021 -0400 update corpus page to show list of documents in corpus and allows users to use a modal to update documents in the corpus commit 057d88d Author: Peihua Huang <[email protected]> Date: Fri Jul 16 15:27:17 2021 -0400 update API url and the way we check if a corpus contains a document commit a8d3e18 Author: Peihua Huang <[email protected]> Date: Thu Jul 15 16:32:18 2021 -0400 update the way documents list is passed into api commit f3dc588 Author: Peihua Huang <[email protected]> Date: Thu Jul 15 16:07:09 2021 -0400 update function naming typo commit b474af5 Merge: 593547e 9b828d9 Author: Peihua Huang <[email protected]> Date: Thu Jul 15 15:26:21 2021 -0400 Merge branch 'main' into create_single_corpus_component commit 593547e Author: Peihua Huang <[email protected]> Date: Wed Jul 14 15:46:04 2021 -0400 added list of documents that can be added to corpus commit 2f2209a Author: Peihua Huang <[email protected]> Date: Wed Jul 14 14:55:55 2021 -0400 added path and get route to backend commit 7eed0c0 Author: Peihua Huang <[email protected]> Date: Wed Jul 14 14:47:27 2021 -0400 created corpus component
This PR follows PR #43 and it:
CorpusReact component, displaying thetitle,description, anddocumentsof aCorpusinstancedocumentsin theCorpusinstance