-
Notifications
You must be signed in to change notification settings - Fork 6
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
CARDS-603 #1790
base: dev
Are you sure you want to change the base?
Conversation
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.
Some really quick notes below, will have more suggestions once these are addressed.
body: checkoutForm | ||
}); | ||
}, | ||
questionnaireData: (id) => { |
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.
functions should have an action in their name for clarity, e.g. fetchQuestionnaire
questionnaireData: (id) => { | ||
return fetch(`/Questionnaires/${id}.deep.json`) | ||
}, | ||
fetchData: (data) => { |
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.
fetchResourceJson
seems more appropriate as you're passing a resource path and obtaining a JSON
reorderEntry: // CALL SlingPostServlet | ||
(entryPath, destinationIndex) => { | ||
// sibling level reordering | ||
let reorderForm = new FormData(); |
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.
reorderedForm
?
} | ||
} | ||
|
||
export function ReorderEntryModal(props) { |
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.
MoveEntryModal
}; | ||
return ( | ||
<> | ||
<Tooltip title="Click to reorder entry" // onClick={() => console.log('activate modal')} |
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.
No "Click to". I'm not sure a tooltip is necessary.
<Box textAlign="center" style={{ display: dndState.enabled ? 'block' : 'none' }}> | ||
<Tooltip title="Drag and drop to reorder"> | ||
<IconButton size="large" {...provided.dragHandleProps}> | ||
<DragIndicatorIcon /> | ||
</IconButton> | ||
</Tooltip> | ||
</Box> | ||
<EntryType | ||
data={value} | ||
model={typeModels?.[stripCardsNamespace(value['jcr:primaryType'])]} | ||
onActionDone={onActionDone} | ||
classes={classes} | ||
/> |
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.
Instead of wrapping the entry card in a box, could we pass a parameter informing it that it is draggable, and based on that parameter display the drag handler directly inside the QuestionnaireItemCard?
<div | ||
ref={provided.innerRef} | ||
// Change background when dragging to indicate to user | ||
style={{ backgroundColor: snapshot.isDraggingOver ? 'lightblue' : 'lightgrey' }} |
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.
always use theme.palette
colors
// .catch(handleError) | ||
.finally(() => { | ||
handleDataChange() | ||
dndDispatch({ type: 'setSnackbar', payload: { open: true, message: "Updated questionnaire" } }) |
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.
dndDispatch({ type: 'setSnackbar', payload: { open: true, message: "Updated questionnaire" } }) | |
dndDispatch({ type: 'setSnackbar', payload: { open: true, message: "Updated" } }) |
// change background colour if dragging | ||
borderStyle: isDragging ? "dashed" : undefined, | ||
borderWidth: isDragging ? "2px" : undefined, | ||
backgroundColor: isDragging ? "lightblue" : undefined, |
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.
theme.palette
colors please
let [data, setData] = useState(); | ||
console.log(data) | ||
let [questionnaireTitle, setQuestionnaireTitle] = useState(); | ||
let [actionsMenu, setActionsMenu] = useState(null); | ||
let [error, setError] = useState(); |
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 believe most of our code uses space after [
and before ]
for better readability.
In general, please avoid whitespace changes that are not directly relevant to the task. E.g. changing indentation to move some code inside a loop is ok, changing indentation in unrelated code should be avoided. If you encounter indentation / white spacing that is not according to the code style, it can be fixed in a separate PR. This enables us to focus on meaningful changes when examining the history.
… dragindicator icon styling wip
…tion context for populating move modal (needs other entry types); drag and drop state + snackbar moved out into context provider with access to questionnaire context
…working for preselected and selectable reorderSource entry
…ties not in spec object and use autocomplete in MoveEntryModal; remove dnd at questionnaire component entry level;
…omponent can be labelled for any entry type
…ed top level component and wrapped around preview and editor. entryData for questionnaire fills initial tree and subsequent updates are tracked in the tree (original entryData stored for its own card component). Modal components for moving by selection and dnd
To test:
COMPLETE
- Single level dnd (no tree structure)
IN PROGRESS
- BUG (styling): (disable on expanded questionnaire) creating new entry collapses and hides drag handlers
- Constructing tree context of entries to search paths to check which are viable parents to reorder an element to
ON HOLD
- Move snackbar into provider
- Styling draggable handler into avatar of item card (currently appears above element in new line)
- Styling margins for nested draggable elements
QUESTIONS
- Can we assume data entries that have fields that are objects are further entries? (i.e. to parse a list of questions, can we construct based on the typing of their fields? otherwise will need to parse based on path(?))
- Should pom.xml explicitly use python3? (Python2 module doesn’t have ‘fullmatch’)
WAITING
-