Added Text Search form and query creation to workspaces #272
Conversation
|
Save/Search buttons should have margin/spacing between them |
| onCompleted: data => { | ||
| save({ | ||
| variables: { | ||
| id: workspaceId, | ||
| attributes: { | ||
| queries: [data.createMetacard.id], | ||
| metacard_type: 'workspace', | ||
| }, | ||
| }, | ||
| refetchQueries: ['WorkspaceById'], | ||
| }), | ||
| setQueryId(data.createMetacard.id) |
There was a problem hiding this comment.
You probably want to wait to set the query id until after it's been added to the workspace.
| onCompleted: data => { | |
| save({ | |
| variables: { | |
| id: workspaceId, | |
| attributes: { | |
| queries: [data.createMetacard.id], | |
| metacard_type: 'workspace', | |
| }, | |
| }, | |
| refetchQueries: ['WorkspaceById'], | |
| }), | |
| setQueryId(data.createMetacard.id) | |
| onCompleted: async data => { | |
| await save({ | |
| variables: { | |
| id: workspaceId, | |
| attributes: { | |
| queries: [data.createMetacard.id], | |
| metacard_type: 'workspace', | |
| }, | |
| }, | |
| refetchQueries: ['WorkspaceById'], | |
| }), | |
| setQueryId(data.createMetacard.id) |
| }, | ||
| }) | ||
| } catch (err) { | ||
| //eslint-disable-next-line |
There was a problem hiding this comment.
If I'm not mistaken, console logs get stripped when the build is ran in production mode, so if that's the case you guys should remove the eslint no-console rule so you don't have to add these comments everywhere.
There was a problem hiding this comment.
I think it's nice to have these explicitly ignored because some people use console logs to debug and we don't have a strict review process, so I worry random console logs would get in
| const onClickSearch = async () => { | ||
| await onCreate(true) |
There was a problem hiding this comment.
Don't have to wait anymore because nothing is happening afterwards.
| const onClickSearch = async () => { | |
| await onCreate(true) | |
| const onClickSearch = () => { | |
| onCreate(true) |
There was a problem hiding this comment.
Also looks like you don't need the true argument because onCreate doesn't take any arguments
| @@ -123,9 +123,9 @@ const typeDefs = ` | |||
| createMetacard(attrs: MetacardAttributesInput!): MetacardAttributes | |||
| saveMetacard(id: ID!, attributes: MetacardAttributesInput!): MetacardAttributes | |||
|
|
|||
| # TBD: Should only be used when... | |||
| # TBD: Should only be used when updating assocations with IDs | |||
There was a problem hiding this comment.
| # TBD: Should only be used when updating assocations with IDs | |
| # Should only be used when updating associations with IDs or other expanded fields (i.e. lists, queries) where you need to set raw values but the schema would otherwise not allow you. |
| @@ -5,9 +5,13 @@ import Tab from '@material-ui/core/Tab' | |||
| import Tabs from '@material-ui/core/Tabs' | |||
| import Typography from '@material-ui/core/Typography' | |||
| import gql from 'graphql-tag' | |||
There was a problem hiding this comment.
I think we should break the workspaces (index cards) routes and the workspace route into two different files. This one is getting too big now and it's hard to see the separation
There was a problem hiding this comment.
Agreed, though it might be best as a separate issue to manage the scope of these changes.
There was a problem hiding this comment.
That's fair. I think it would be a good idea to prioritize this before adding more functionality to workspaces going forward
| variables: { | ||
| attrs: { | ||
| title: title, | ||
| cql: cql, |
There was a problem hiding this comment.
I think we should be storing the filterTree and not the cql
| } | ||
| } | ||
| }, | ||
| [queryIdToRun, data, onSearch] |
There was a problem hiding this comment.
It seems like only queryIdToRun needs to be in this array
| value: textValue, | ||
| } | ||
|
|
||
| const cql = transformFilterToCQL(filter) |
There was a problem hiding this comment.
We shouldn't have cql anywhere on the frontend. graphql should be responsible for any conversions
| onCompleted: data => { | ||
| save({ |
There was a problem hiding this comment.
We should be using the update option instead and writing to the apollo cache to avoid the need to add refetchQueries
There was a problem hiding this comment.
refetchQueries is a nice to thing have when you can use it. Normally in our system we can't because our data is stored in SOLR and with soft commit times, a refetchQueries would normally just return old data. However, when you query by id, solr is guaranteed to give you up to date data. In this specific case, the refetchQueries he's running is on a workspaceById query which means it will be fine.
I am of the opinion that if you don't have to write this goofy/complicated cache updating logic, you shouldn't have to.
| ` | ||
|
|
||
| const useCreateQuery = workspaceId => { | ||
| const [queryId, setQueryId] = React.useState('') |
There was a problem hiding this comment.
useCreateQuery shouldn't need to know about the queryId
There was a problem hiding this comment.
Not sure I follow. The query ID is needed when the workspace is updated on line 105. It's also used to execute the useEffect in the case that the user hit Search after creating the query itself.
There was a problem hiding this comment.
useCreateQuery having access to the queryId feels like a code smell to me. I think the logic of associating a query to a workspace should be handled by the graphql side
| import { getIn } from 'immutable' | ||
| import React, { useState } from 'react' | ||
| import loadable from 'react-loadable' | ||
| import SaveAltIcon from '@material-ui/icons/SaveAlt' |
There was a problem hiding this comment.
Why did you choose to use this icon instead of the ordinary Save (floppy disc) icon? Maybe it's just me, but this icon makes me think that something is going to be downloaded, not saved when I see it
Adds Text Search form creation to workspace view and implements search and save buttons. This is a somewhat interim PR in that the inital creation view will be updated to include basic / advanced / custom, but this is a good starting point. Could use feedback on the form style.
tied to #244