Feature/persistant columns #75
Conversation
| @@ -8,6 +8,7 @@ import { PersistedConfig, PersistedConfigKeys } from "../services/PersistentConf | |||
| import interaction, { InteractionStateBranch } from "./interaction"; | |||
| import metadata, { MetadataStateBranch } from "./metadata"; | |||
| import selection, { SelectionStateBranch } from "./selection"; | |||
| import Annotation from "../entity/Annotation"; | |||
There was a problem hiding this comment.
This import belongs with the above grouping (so as a new line above line 6)
There was a problem hiding this comment.
Moving this import causes the app to go blank. I am completely lost on why.
There was a problem hiding this comment.
Pulled locally and tried this as well, also caused a load error for me when put anywhere from line 8 up:
Uncaught ReferenceError: Cannot access 'Annotation' before initialization
| console.error("Failed to fetch annotations", err); | ||
| } | ||
|
|
||
| dispatch(receiveAnnotations(annotations)); |
There was a problem hiding this comment.
We don't want to dispatch this if we don't have annotations & the defaults wouldn't be able to render anyway without them. In that case it seems like it would be best to have the catch statement look like:
console.error("Failed to fetch annotations", err);
done();
return;
and the dispatch(receiveAnnotations(annotations)); should move into the try block as the last line in it.
There was a problem hiding this comment.
resolved in f3b21ab
| @@ -157,15 +157,19 @@ export const DESELECT_DISPLAY_ANNOTATION = makeConstant( | |||
| ); | |||
|
|
|||
| export interface DeselectDisplayAnnotationAction { | |||
| payload: Annotation | Annotation[]; | |||
| payload: { | |||
There was a problem hiding this comment.
I thought this made it a little easer to read mirroring the SelectDisplayAnnotationAction so that both have action.payload.annotation as the annotation rather than action.payload for one and action.payload.annotation for another
| @@ -123,9 +123,9 @@ export default makeReducer<SelectionStateBranch>( | |||
| sortColumn: action.payload, | |||
| }), | |||
| [DESELECT_DISPLAY_ANNOTATION]: (state, action) => { | |||
| const displayAnnotations = without( | |||
There was a problem hiding this comment.
Isn't this what without was doing? Why the change?
There was a problem hiding this comment.
this was the source of the reselect bug I was talking about, for some reason the annotations returned by the persist are slightly different and this dont get removed with the without function. The same is true for comparing the objects directly. This is what i was saying about not liking my solution as much.
There was a problem hiding this comment.
Looks like lodash's without function doesn't support this kind of object comparison -- unless the objects share a reference, they aren't considered equal. I'm curious if this use of without worked when the payload to deselectDisplayAnnotation was previously just annotation instead of {annotation}?
But also am fine with the use of filter here
| [PersistedConfigKeys.UserSelectedApplications]: userSelectedApplications, | ||
| }; | ||
| if (JSON.stringify(appState) !== JSON.stringify(persistentConfigService.getAll())) { | ||
| persistentConfigService.persist({ | ||
| [PersistedConfigKeys.CsvColumns]: csvColumns, | ||
| [PersistedConfigKeys.UserSelectedApplications]: userSelectedApplications, | ||
| ...appState, | ||
| [PersistedConfigKeys.HasUsedApplicationBefore]: hasUsedApplicationBefore, |
There was a problem hiding this comment.
Seems like this should also be in the appState object - this isn't because of your changeset just seems buggy because otherwise isn't this always true?
JSON.stringify(appState) !== JSON.stringify(persistentConfigService.getAll())
If you agree I'd put [PersistedConfigKeys.HasUsedApplicationBefore]: true into the appState object.
There was a problem hiding this comment.
resolved in f3b21ab
| return; | ||
| } | ||
|
|
||
| if (!displayAnnotations.length) { |
There was a problem hiding this comment.
When I initially loaded this branch locally, got an error on load that caused the app to fail because displayAnnotations was set to [undefined] which technically has length 1. Brian and I were able to special case around it temporarily, but since then haven't been able to replicate the error. Wanted to check if anyone else (@SeanLeRoy ?) has seen this bug locally?
The purpose of this PR is to resolve #57 and introduce persistent columns (Display Annotations) for users after closing the app.
Testing procedure:
Initial feature test
Spam testing
Testing with annotation hierarchy