Skip to content
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

Add rename variable feature #733

Closed
wants to merge 38 commits into from
Closed

Add rename variable feature #733

wants to merge 38 commits into from

Conversation

wwqrd
Copy link
Contributor

@wwqrd wwqrd commented Sep 8, 2021

Updates codebook and inline variable selection to include a rename variable dialog.

Delete button in codebook has been changed to an icon to create more room for the rename button.

Rename dialog
dialog

Codebook view
codebook_rename

Inline view
inline_rename

Resolves #732

@wwqrd wwqrd requested a review from rebeccamadsen September 9, 2021 15:35
@rebeccamadsen
Copy link
Contributor

Renaming variables is generally working well for me! There were just a few places that editing the variable name was allowed, but it did not trigger a save for the stage so didn't take:

  • additional variables
  • layout variables (sociogram and narrative)
  • toggle variables (sociogram).

And there are a few places that variables cannot be renamed but maybe could be:

  • edge selection (dyad census, tie-strength, sociogram)
  • group variable (narrative)

Editing from the codebook worked for all the variables, though!

When navigating to the codebook, I saw these three warnings:

index.js:1 Warning: Failed prop type: The prop `type` is marked as required in `Variables`, but its value is `undefined`.
    in Variables (created by withProps(Variables))
    in withProps(Variables) (created by withStateHandlers(withProps(Variables)))
    in withStateHandlers(withProps(Variables)) (created by withHandlers(withStateHandlers(withProps(Variables))))
    in withHandlers(withStateHandlers(withProps(Variables))) (created by ConnectFunction)
    in ConnectFunction (created by EgoType)
    in div (created by EgoType)
    in div (created by EgoType)
    in EgoType (created by ConnectFunction)
    in ConnectFunction (created by Codebook)
    in div (created by CodebookCategory)
    in div (created by Section)
    in Section (created by CodebookCategory)
    in CodebookCategory (created by Codebook)
    in div (created by Codebook)
    in Codebook (created by ConnectFunction)
    in ConnectFunction (created by CodebookScreen)
    in div (created by Layout)
    in Layout (created by CodebookScreen)
    in CardErrorBoundary (created by Screen)
    in div (created by Screen)
    in div (created by Screen)
    in div (created by Screen)
    in Screen (created by withContext(Screen))
    in withContext(Screen) (created by withState(withContext(Screen)))
    in withState(withContext(Screen)) (created by CodebookScreen)
    in CodebookScreen (created by Screens)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by Screens)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by Screens)
    in Screens (created by () => `Window(${getDisplayName(WrappedComponent)})`)
    in () => `Window(${getDisplayName(WrappedComponent)})` (created by getContext(() => `Window(${getDisplayName(WrappedComponent)})`))
    in getContext(() => `Window(${getDisplayName(WrappedComponent)})`) (created by ConnectFunction)
    in ConnectFunction (created by Routes)
    in Routes (created by AppView)
    in AppErrorBoundary (created by AppView)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by AppView)
    in AppView (created by Context.Consumer)
    in Route (created by ViewManager)
    in Switch (created by ViewManager)
    in Router (created by HashRouter)
    in HashRouter (created by ViewManager)
    in ViewManager
    in PersistGate
    in Provider
index.js:1 Warning: Failed prop type: The prop `type` is marked as required in `ControlsColumn`, but its value is `undefined`.
    in ControlsColumn (created by Variables)
    in Variables (created by withProps(Variables))
    in withProps(Variables) (created by withStateHandlers(withProps(Variables)))
    in withStateHandlers(withProps(Variables)) (created by withHandlers(withStateHandlers(withProps(Variables))))
    in withHandlers(withStateHandlers(withProps(Variables))) (created by ConnectFunction)
    in ConnectFunction (created by EgoType)
    in div (created by EgoType)
    in div (created by EgoType)
    in EgoType (created by ConnectFunction)
    in ConnectFunction (created by Codebook)
    in div (created by CodebookCategory)
    in div (created by Section)
    in Section (created by CodebookCategory)
    in CodebookCategory (created by Codebook)
    in div (created by Codebook)
    in Codebook (created by ConnectFunction)
    in ConnectFunction (created by CodebookScreen)
    in div (created by Layout)
    in Layout (created by CodebookScreen)
    in CardErrorBoundary (created by Screen)
    in div (created by Screen)
    in div (created by Screen)
    in div (created by Screen)
    in Screen (created by withContext(Screen))
    in withContext(Screen) (created by withState(withContext(Screen)))
    in withState(withContext(Screen)) (created by CodebookScreen)
    in CodebookScreen (created by Screens)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by Screens)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by Screens)
    in Screens (created by () => `Window(${getDisplayName(WrappedComponent)})`)
    in () => `Window(${getDisplayName(WrappedComponent)})` (created by getContext(() => `Window(${getDisplayName(WrappedComponent)})`))
    in getContext(() => `Window(${getDisplayName(WrappedComponent)})`) (created by ConnectFunction)
    in ConnectFunction (created by Routes)
    in Routes (created by AppView)
    in AppErrorBoundary (created by AppView)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by AppView)
    in AppView (created by Context.Consumer)
    in Route (created by ViewManager)
    in Switch (created by ViewManager)
    in Router (created by HashRouter)
    in HashRouter (created by ViewManager)
    in ViewManager
    in PersistGate
    in Provider
index.js:1 Warning: Failed prop type: The prop `type` is marked as required in `RenameVariableControl`, but its value is `undefined`.
    in RenameVariableControl (created by ConnectFunction)
    in ConnectFunction (created by ControlsColumn)
    in ControlsColumn (created by Variables)
    in td (created by Variables)
    in tr (created by Variables)
    in tbody (created by Variables)
    in table (created by Variables)
    in div (created by Variables)
    in Variables (created by withProps(Variables))
    in withProps(Variables) (created by withStateHandlers(withProps(Variables)))
    in withStateHandlers(withProps(Variables)) (created by withHandlers(withStateHandlers(withProps(Variables))))
    in withHandlers(withStateHandlers(withProps(Variables))) (created by ConnectFunction)
    in ConnectFunction (created by EgoType)
    in div (created by EgoType)
    in div (created by EgoType)
    in EgoType (created by ConnectFunction)
    in ConnectFunction (created by Codebook)
    in div (created by CodebookCategory)
    in div (created by Section)
    in Section (created by CodebookCategory)
    in CodebookCategory (created by Codebook)
    in div (created by Codebook)
    in Codebook (created by ConnectFunction)
    in ConnectFunction (created by CodebookScreen)
    in div (created by Layout)
    in Layout (created by CodebookScreen)
    in CardErrorBoundary (created by Screen)
    in div (created by Screen)
    in div (created by Screen)
    in div (created by Screen)
    in Screen (created by withContext(Screen))
    in withContext(Screen) (created by withState(withContext(Screen)))
    in withState(withContext(Screen)) (created by CodebookScreen)
    in CodebookScreen (created by Screens)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by Screens)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by Screens)
    in Screens (created by () => `Window(${getDisplayName(WrappedComponent)})`)
    in () => `Window(${getDisplayName(WrappedComponent)})` (created by getContext(() => `Window(${getDisplayName(WrappedComponent)})`))
    in getContext(() => `Window(${getDisplayName(WrappedComponent)})`) (created by ConnectFunction)
    in ConnectFunction (created by Routes)
    in Routes (created by AppView)
    in AppErrorBoundary (created by AppView)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by AppView)
    in AppView (created by Context.Consumer)
    in Route (created by ViewManager)
    in Switch (created by ViewManager)
    in Router (created by HashRouter)
    in HashRouter (created by ViewManager)
    in ViewManager
    in PersistGate
    in Provider

This showed up when navigating to a name generator stage and an ego stage, and an alter form stage. So probably unrelated?

index.js:1 Warning: Failed prop type: Invalid prop `item` of type `object` supplied to `OrderedList`, expected `function`.
    in OrderedList (created by sortableList(OrderedList))
    in sortableList(OrderedList) (created by withHandlers(sortableList(OrderedList)))
    in withHandlers(sortableList(OrderedList)) (created by renameProp(withHandlers(sortableList(OrderedList))))
    in renameProp(withHandlers(sortableList(OrderedList))) (created by defaultProps(renameProp(withHandlers(sortableList(OrderedList)))))
    in defaultProps(renameProp(withHandlers(sortableList(OrderedList)))) (created by ConnectedFieldArray)
    in ConnectedFieldArray (created by ConnectFunction)
    in ConnectFunction (created by Connect(ConnectedFieldArray))
    in Connect(ConnectedFieldArray) (created by FieldArray)
    in FieldArray (created by Context.Consumer)
    in Hoc (created by FieldArray)
    in FieldArray (created by Validated)
    in Validated (created by EditableList)
    in div (created by EditableList)
    in div (created by EditableList)
    in div (created by l)
    in l (created by EditableList)
    in div (created by Section)
    in Section (created by EditableList)
    in EditableList (created by ConnectFunction)
    in ConnectFunction (created by withHandlers(Connect(EditableList)))
    in withHandlers(Connect(EditableList)) (created by withStateHandlers(withHandlers(Connect(EditableList))))
    in withStateHandlers(withHandlers(Connect(EditableList))) (created by ConnectFunction)
    in ConnectFunction (created by defaultProps(Connect(withStateHandlers(withHandlers(Connect(EditableList))))))
    in defaultProps(Connect(withStateHandlers(withHandlers(Connect(EditableList))))) (created by defaultProps(defaultProps(Connect(withStateHandlers(withHandlers(Connect(EditableList)))))))
    in defaultProps(defaultProps(Connect(withStateHandlers(withHandlers(Connect(EditableList)))))) (created by Form)
    in div (created by Section)
    in Section (created by Form)
    in Form (created by withProps(Form))
    in withProps(Form) (created by withProps(withProps(Form)))
    in withProps(withProps(Form)) (created by withHandlers(withProps(withProps(Form))))
    in withHandlers(withProps(withProps(Form))) (created by ConnectFunction)
    in ConnectFunction (created by ConnectFunction)
    in ConnectFunction (created by Editor)
    in div (created by Layout)
    in Layout (created by Editor)
    in form (created by Form)
    in Form (created by Context.Consumer)
    in Hoc (created by Form)
    in Form (created by Editor)
    in Editor (created by ConnectFunction)
    in ConnectFunction (created by Form(Connect(Editor)))
    in Form(Connect(Editor)) (created by ConnectFunction)
    in ConnectFunction (created by Connect(Form(Connect(Editor))))
    in Connect(Form(Connect(Editor))) (created by ReduxForm)
    in ReduxForm (created by Context.Consumer)
    in Hoc (created by ReduxForm)
    in ReduxForm (created by withStateHandlers(ReduxForm))
    in withStateHandlers(ReduxForm) (created by StageEditor)
    in StageEditor (created by withHandlers(StageEditor))
    in withHandlers(StageEditor) (created by ConnectFunction)
    in ConnectFunction (created by ConnectFunction)
    in ConnectFunction (created by defaultProps(Connect(Connect(withHandlers(StageEditor)))))
    in defaultProps(Connect(Connect(withHandlers(StageEditor)))) (created by Screen)
    in CardErrorBoundary (created by Screen)
    in div (created by Screen)
    in div (created by Screen)
    in div (created by Screen)
    in Screen (created by withContext(Screen))
    in withContext(Screen) (created by withState(withContext(Screen)))
    in withState(withContext(Screen)) (created by EditorScreen)
    in EditorScreen (created by ConnectFunction)
    in ConnectFunction (created by withProps(Connect(EditorScreen)))
    in withProps(Connect(EditorScreen)) (created by withHandlers(withProps(Connect(EditorScreen))))
    in withHandlers(withProps(Connect(EditorScreen))) (created by ConnectFunction)
    in ConnectFunction (created by Screens)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by Screens)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by Screens)
    in Screens (created by () => `Window(${getDisplayName(WrappedComponent)})`)
    in () => `Window(${getDisplayName(WrappedComponent)})` (created by getContext(() => `Window(${getDisplayName(WrappedComponent)})`))
    in getContext(() => `Window(${getDisplayName(WrappedComponent)})`) (created by ConnectFunction)
    in ConnectFunction (created by Routes)
    in Routes (created by AppView)
    in AppErrorBoundary (created by AppView)
    in div (created by ForwardRef(MotionComponent))
    in ForwardRef(MotionComponent) (created by AppView)
    in AppView (created by Context.Consumer)
    in Route (created by ViewManager)
    in Switch (created by ViewManager)
    in Router (created by HashRouter)
    in HashRouter (created by ViewManager)
    in ViewManager
    in PersistGate
    in Provider

@wwqrd wwqrd mentioned this pull request Sep 28, 2021
3 tasks
@wwqrd
Copy link
Contributor Author

wwqrd commented Sep 28, 2021

Thanks for this feedback!

Renaming variables is generally working well for me! There were just a few places that editing the variable name was allowed, but it did not trigger a save for the stage so didn't take:

This was an interesting one, as normally that's controlled by the form "dirty" state, but variable names are edited directly in the protocol. I updated it to detect when the locus point (a pointer to the last place in the protocol history before the screen was opened), is no longer the last point in the protocol history. This feature is used for the "cancel" button, so I think it's reasonably coherent conceptually. However, it does look a bit weird when you have a prompt open, which has it's own save/cancel buttons, and the main screen finish editing button is displayed - any thoughts?

Additionally in working on this feature I discovered quite a big bug in the history/rewind feature (a redux enhancer). It was considering any action as a change to snapshot instead of only changes to the protocol (the intent), that means the entire protocol state was being duplicated multiple times for any redux-form actions (which there are quite a lot of!). Performance has been improved, and the history object is much slimmer. I am concerned that there could be other knock on effects, but there is no reason for that to be the case unless there is some unusual behaviour somewhere that I've forgotten about.

And there are a few places that variables cannot be renamed but maybe could be:

I've added this feature to the narrative group variable.

I might be misunderstanding, but the edge selection doesn't involve variables (though the UI is very similar so it does seem like a similar feature would fit here).

@rebeccamadsen
Copy link
Contributor

I agree that it looks a bit weird when the "finished editing" button pops up while still having the prompt open. More than once I clicked on "finished editing" instead of the "save and close". Sounds like a tricky issue though! It definitely needs to trigger a save, but it'd be nice not to do so until the prompt closes, like the other prompt edits would. Is there a way to do that, or is that too involved?

Good find with the history/rewind bug! I'll let you know if I come across any oddities. I haven't yet, so fingers crossed.

Good point about how the edge selection is different from variables...at the level of the node type. The UI similarities made me forget that distinction. It sounds to me like a separate issue if we decide we want that also.

@jthrilly
Copy link
Member

jthrilly commented Oct 4, 2021

It definitely needs to trigger a save, but it'd be nice not to do so until the prompt closes, like the other prompt edits would. Is there a way to do that, or is that too involved?

Any thoughts, @wwqrd?

@wwqrd
Copy link
Contributor Author

wwqrd commented Oct 5, 2021

It definitely needs to trigger a save, but it'd be nice not to do so until the prompt closes, like the other prompt edits would. Is there a way to do that, or is that too involved?

This change has also retrospectively impacted some other features - like inline variable creation (also seen as a change in protocol)

I'd be tempted to find a way to disable those controls until the sub window is closed.

@jthrilly
Copy link
Member

jthrilly commented Oct 8, 2021

I'm working on something for this, so please don't merge for now.

@wwqrd
Copy link
Contributor Author

wwqrd commented Dec 20, 2021

Superseded by: #738

@wwqrd wwqrd closed this Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename variables
3 participants