[ENH] Implemented ColumnAnnotationCard component
#59
Conversation
Reviewer's Guide by SourceryThis pull request introduces the Updated class diagram for the Columns typeclassDiagram
class Columns {
<<interface>>
[key: number]: Column
}
class Column {
header: string
description: string | null
dataType: 'Categorical' | 'Continuous' | null
standardizedVariable: StandardizedVarible | null
}
class StandardizedVarible {
<<interface>>
identifier: string
}
Columns -- Column : contains
Column -- StandardizedVarible : has *
Class diagram for ColumnAnnotationCard componentclassDiagram
class ColumnAnnotationCard {
id: number
header: string
description: string | null
dataType: 'Categorical' | 'Continuous' | null
standardizedVariable: StandardizedVarible | null
standardizedVariableOptions: StandardizedVaribles
onDescriptionChange: (columnId: number, newDescription: string | null) => void
onDataTypeChange: (columnId: number, newDataType: 'Categorical' | 'Continuous' | null) => void
onStandardizedVariableChange: (columnId: number, newStandardizedVariable: StandardizedVarible | null) => void
}
class StandardizedVarible {
identifier: string
}
class StandardizedVaribles {
[key: string]: StandardizedVarible
}
ColumnAnnotationCard -- StandardizedVarible : uses
ColumnAnnotationCard -- StandardizedVaribles : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Deploy Preview for staging-annotation ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
rmanaem
added
the
pr-minor
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
- Coverage 81.77% 79.79% -1.99%
==========================================
Files 20 21 +1
Lines 225 292 +67
Branches 46 56 +10
==========================================
+ Hits 184 233 +49
- Misses 41 59 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hey @rmanaem, thanks for the PR - and sorry it took so long to review.
A little like last time: the UI looks amazing, really very nice and close to what we would want in the final product. My main concerns are with the readability and modularity of the components - and how that will affect our ability to maintain and add things later.
There is also a bug with the standardized variables that happens because of duplicated state. But ultimately I think this is also related to the length and relative complexity of the components that make it harder to see what's going on and to reason about.
So I would say we factor things out, make the components very focused on single tasks. I would also say: be very hesitant to introduce complexity just for better UX at this point. E.g. the description editing UI is pretty, but you had to do a lot of work over a simple and less pretty textbox. I strongly suggest we leave such things for later and focus on getting the key functionality going now.
|
|
||
| const totalPages = Math.ceil(columnEntries.length / columnsPerPage); | ||
|
|
||
| const handlePageChange = (_: React.ChangeEvent<unknown>, page: number) => { |
There was a problem hiding this comment.
🍒 handlePageChange is really concerned with handling the paginated views of the column table/card element, correct? I'd say we make that explicit to differentiate from navigation. Maybe handlePaginationChange here, and then also make sure we have a distinct name for "stages" or "views" that is different from pages.
| updateColumnDescription: (columnId: number, description: string | null) => { | ||
| set((state) => ({ | ||
| columns: R.assocPath([columnId, 'description'], description, state.columns), | ||
| })); | ||
| }, | ||
|
|
||
| updateColumnDataType: (columnId: number, dataType: 'Categorical' | 'Continuous' | null) => { | ||
| set((state) => ({ | ||
| columns: R.assocPath([columnId, 'dataType'], dataType, state.columns), | ||
| })); | ||
| }, | ||
|
|
||
| updateColumnStandardizedVariable: ( | ||
| columnId: number, | ||
| standardizedVariable: StandardizedVarible | null | ||
| ) => { | ||
| set((state) => ({ | ||
| columns: R.assocPath([columnId, 'standardizedVariable'], standardizedVariable, state.columns), | ||
| })); | ||
| }, | ||
|
|
||
| // Data dictionary |
There was a problem hiding this comment.
I may have missed this before, but why did you go with Ramda over Immer here? My sense on a first read of https://zustand.docs.pmnd.rs/guides/updating-state#deeply-nested-object and some other docs on this is that Immer is more straightforward to read and has more docs / tutorials out there.
| ) => { | ||
| updateColumnStandardizedVariable(columnId, newStandardizedVariable); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="flex flex-col items-center"> |
There was a problem hiding this comment.
I think there are two custom hooks in here you could factor out and improve readability: one for handling the pagination (and that might be reused in other places), and one for handling updates to the columns themselves.
| {isEditingDescription ? ( | ||
| <div className="flex flex-col items-center gap-4 md:flex-row"> | ||
| <TextField | ||
| data-cy={`${id}-column-annotation-card-description-input`} | ||
| fullWidth | ||
| multiline | ||
| rows={3} | ||
| value={editedDescription || ''} | ||
| onChange={(e) => setEditedDescription(e.target.value)} | ||
| variant="outlined" | ||
| label="Description" | ||
| className="flex-1" | ||
| /> | ||
| <Fab | ||
| data-cy={`${id}-column-annotation-card-save-description-button`} | ||
| color="secondary" | ||
| aria-label="save" | ||
| onClick={handleSaveDescription} | ||
| size="small" | ||
| className="mt-4 md:mt-0" | ||
| > | ||
| <SaveIcon /> | ||
| </Fab> | ||
| </div> | ||
| ) : ( | ||
| <div className="flex flex-col items-center gap-4 md:flex-row"> | ||
| <div className="flex-1"> | ||
| <Typography variant="subtitle1" className="mb-2 text-gray-700"> | ||
| Description: | ||
| </Typography> | ||
| <Typography | ||
| data-cy={`${id}-column-annotation-card-description`} | ||
| variant="body1" | ||
| className="text-gray-700" | ||
| > | ||
| {description || 'No description provided.'} | ||
| </Typography> | ||
| </div> | ||
| <Fab | ||
| data-cy={`${id}-column-annotation-card-edit-description-button`} | ||
| color="primary" | ||
| aria-label="edit" | ||
| onClick={handleEditDescription} | ||
| size="small" | ||
| className="mt-4 md:mt-0" | ||
| > | ||
| <EditIcon /> | ||
| </Fab> | ||
| </div> | ||
| )} | ||
|
|
||
| <div className="mt-4 flex flex-col items-center gap-4 md:flex-row"> |
There was a problem hiding this comment.
This is looking pretty, but it's also a whole lot to wrap your head around. I think the whole "Description editor" section should probably be it's own component.
| const [selectedStandardizedVariableKey, setSelectedStandardizedVariableKey] = useState< | ||
| string | null | ||
| >(standardizedVariable ? standardizedVariable.identifier : null); | ||
|
|
There was a problem hiding this comment.
That doesn't seem right. You are already keeping the "selectedStandardizedVariable" inside the store. Why do you need to also handle this in state?
There was a problem hiding this comment.
In fact there seems to be a bug where on initial select, the variable name uses the key of the Standardized Variable (i.e. human readable name), and when you navigate away and back to the component, it instead shows the identifier. I'm pretty sure this bug is related to duplicated state here - not fully clear yet how though
There was a problem hiding this comment.
Aha, yes, here you set the "value" to standardizedVariable.identifier when instead we would probably want to use the label instead. I left another comment on types.ts but I think this is caused in part by the fact that you (I believe) use the key in StandardizedVariable**S** to encode the label, and then only add in the identifier in each given StandardizedVariable instance. The reason we don't see that the dropdown value gets passed the identifier is because of the duplicated and conflicting local component state that is set to the "label" in parallel. Only when you navigate away and back does the store state override the component state and you see the identifier show up.
So bottom line: let's keep single source of truth and let's keep all relevant info (label + identifier) inside the StandardizedVariable object.
| @@ -35,3 +37,11 @@ export interface DataDictionary { | |||
| }; | |||
| }; | |||
| } | |||
|
|
|||
| export interface StandardizedVarible { | |||
| identifier: string; | |||
There was a problem hiding this comment.
I think StandardizedVarible should also have a .name or .label attribute. Otherwise you entirely rely on the string key in the StandardizedVaribles object for the human readable label - and that's no good.
| } | ||
|
|
||
| export interface StandardizedVaribles { | ||
| [key: string]: StandardizedVarible; |
There was a problem hiding this comment.
similar to above: I would advise against using the name of the StandardizedVarible as the key. Either a numeric key or the identifier or whatever. But something that is 1) guaranteed to be unique, and 2) used to look up important information (like the label or identifier), but not to encode that info directly
There was a problem hiding this comment.
also minor point: Can we find a more distinct name for this object of several StandardizedVarible? That's going to confuse someone pretty certainly. How about StandardizedVaribleCollection?
| _: React.ChangeEvent<unknown>, | ||
| newValue: string | null | ||
| ) => { | ||
| setSelectedStandardizedVariableKey(newValue); |
There was a problem hiding this comment.
This is incorrect. 1) because you're passing the string "label" but expect a key, 2) and more importantly because you are creating duplicated state here between the component state and the store state (set just 2 lines below)
| it('Fires the onStandardizedVariableChange event handler with the appropriate payload when the standardized variable is changed', () => { | ||
| const spy = cy.spy().as('spy'); | ||
| cy.mount( | ||
| <ColumnAnnotationCard | ||
| id={props.id} | ||
| header={props.header} | ||
| description={props.description} | ||
| dataType={props.dataType} | ||
| standardizedVariable={props.standardizedVariable} | ||
| standardizedVariableOptions={props.standardizedVariableOptions} | ||
| onDescriptionChange={props.onDescriptionChange} | ||
| onDataTypeChange={props.onDataTypeChange} | ||
| onStandardizedVariableChange={spy} | ||
| /> | ||
| ); | ||
| cy.get('[data-cy="1-column-annotation-card-standardized-variable-dropdown"]').type( | ||
| 'age{downarrow}{enter}' | ||
| ); | ||
| cy.get('@spy').should('have.been.calledWith', 1, { identifier: 'nb:Age' }); | ||
| }); |
There was a problem hiding this comment.
Let's add test cases also for "if I set the StandardizedVariable to XYZ in the store, it'll show up with the label" - that would have caught the current bug
There was a problem hiding this comment.
If not feasible for the AnnotationCard because it doesn't use the store directly, then maybe as a test of the parent component where we also load the AnnotationCard child component to ensure that setting the (mocked) store leads to the expected UI rendering outcome of "I now see the label of the Variable selected, not the identifier"
columnAnnotationCardcomponent #53Checklist
This section is for the PR reviewer
[ENH],[FIX],[REF],[TST],[CI],[MNT],[INF],[MODEL],[DOC]) (see our Contributing Guidelines for more info)skip-release(to be applied by maintainers only)Closes #XXXXFor new features:
For bug fixes:
Summary by Sourcery
Implement column annotation functionality, allowing users to view and edit column metadata. Introduce a new
ColumnAnnotationCardcomponent for individual column annotation and integrate it into a paginatedColumnAnnotationview. Enhance the data store to manage and persist column metadata changes.New Features:
ColumnAnnotationCardcomponent that allows users to view and edit column metadata, including description, data type, and standardized variable.ColumnAnnotationcomponent to display a paginated list ofColumnAnnotationCardcomponents.Enhancements:
UploadCardcomponent to use Material UI'sCardHeaderandCardContentcomponents for improved structure and styling.Tests:
ColumnAnnotationCardcomponent.