[ENH] Implemented upload data dictionary component #50
Conversation
Reviewer's Guide by SourceryThis pull request introduces the ability to upload a data dictionary file to enhance column descriptions, and enhances the Sequence diagram for uploading a data dictionary filesequenceDiagram
participant User
participant UploadCard
participant FileUploader
participant DataStore
User->>UploadCard: Clicks to upload data dictionary
UploadCard->>FileUploader: handleClickToUpload()
FileUploader->>User: Selects a .json file
User->>FileUploader: Chooses file
FileUploader->>FileUploader: handleFileUpload(event)
FileUploader->>UploadCard: onFileUpload(file)
UploadCard->>DataStore: processDataDictionaryFile(file)
DataStore->>DataStore: Reads file content
DataStore->>DataStore: Parses JSON to DataDictionary
DataStore->>DataStore: Updates columns with descriptions
DataStore->>DataStore: setUploadedDataDictionaryFileName(fileName)
DataStore->>DataStore: setDataDictionary(dataDictionary)
DataStore-->>UploadCard: Resolves promise
UploadCard-->>User: Updates UI with preview
Updated class diagram for FileUploader componentclassDiagram
class FileUploader {
+displayText: string
+handleClickToUpload: () => void
+handleDrop: (event: React.DragEvent<HTMLDivElement>) => void
+handleDragOver: (event: React.DragEvent<HTMLDivElement>) => void
+handleFileUpload: (event: React.ChangeEvent<HTMLInputElement>) => void
+fileInputRef: React.RefObject<HTMLInputElement>
+allowedFileType: string
+disabled?: boolean
+tooltipContent?: string
}
note for FileUploader "Added 'disabled' and 'tooltipContent' properties"
Updated class diagram for DataStoreclassDiagram
class DataStore {
+dataTable: DataTable
+columns: Columns
+uploadedDataDictionary: DataDictionary
+uploadedDataTableFileName: string | null
+uploadedDataDictionaryFileName: string | null
+setDataTable: (data: DataTable) => void
+initializeColumns: (data: Columns) => void
+setDataDictionary: (data: DataDictionary) => void
+setUploadedDataTableFileName: (fileName: string | null) => void
+setUploadedDataDictionaryFileName: (fileName: string | null) => void
+processDataTableFile: (file: File) => Promise<void>
+processDataDictionaryFile: (file: File) => Promise<void>
}
note for DataStore "Added data dictionary related properties and methods"
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
==========================================
+ Coverage 80.48% 81.09% +0.60%
==========================================
Files 17 19 +2
Lines 123 201 +78
Branches 17 38 +21
==========================================
+ Hits 99 163 +64
- Misses 24 38 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
rmanaem
added
the
pr-minor
There was a problem hiding this comment.
Hey @rmanaem, thanks a lot for the PR! The UI looks really very nice, and the functionality is great. My comments are mainly about the structure of the components and readability. I think now is the best time to set the theme for keeping things clean and easy to understand, it'll only get more complex from here.
So I left comments in a couple of places where I currently find it tricky to follow the component. the main issue is, as they say, debugging is twice as hard as writing code - so if we barely get it while we write it / it's fresh, then we won't get it when it breaks....
Take a look
| @@ -40,5 +40,6 @@ module.exports = { | |||
| */ | |||
| '@stylistic/indent': 'off', | |||
| '@stylistic/comma-dangle': 'off', | |||
| '@typescript-eslint/no-unused-vars': 'off', | |||
There was a problem hiding this comment.
Why is this needed now? Or: please put comment above on why we disable it. Because that seems like a useful linting rule to me
| data-cy={`${key}-expand-collapse-button`} | ||
| > |
There was a problem hiding this comment.
I know this component isn't used yet, but the cypress selector here is not specific enough. Maybe leave a TODO comment here to revisit the selector specificity when this component is to be used
| return ( | ||
| <div> | ||
| <IconButton | ||
| onClick={() => toggleExpand(key)} |
There was a problem hiding this comment.
key is not going to be unique, e.g. many columns will have "Annotations". I won't look much closer since this component isn't use. But I suggest you at least add a big TODO comment above this section to remind us to fix this if we decide to move this component live
| displayText: string; | ||
| handleClickToUpload: () => void; | ||
| handleDrop: (event: React.DragEvent<HTMLDivElement>) => void; | ||
| handleDragOver: (event: React.DragEvent<HTMLDivElement>) => void; | ||
| handleFileUpload: (event: React.ChangeEvent<HTMLInputElement>) => void; | ||
| fileInputRef: React.RefObject<HTMLInputElement>; | ||
| allowedFileType: string; | ||
| disabled?: boolean; | ||
| tooltipContent?: string; | ||
| }) { | ||
| const theme = useTheme(); |
There was a problem hiding this comment.
I liked the interface use before. Why did you go back to defining the types inline here?
| @@ -27,4 +27,20 @@ describe('FileUploader', () => { | |||
| cy.get('[data-cy="upload-area"]').should('be.visible'); | |||
| cy.get('[data-cy="upload-area"]').should('contain', 'Upload your file (.csv)'); | |||
| }); | |||
| it('Should disable the upload area', () => { | |||
There was a problem hiding this comment.
| it('Should disable the upload area', () => { | |
| it('Upload area is inactive when component is disabled', () => { |
| className={`mx-auto max-w-[768px] rounded-3xl border-2 border-dashed p-8 transition-colors ${ | ||
| disabled | ||
| ? 'cursor-not-allowed border-gray-200 bg-gray-100' | ||
| : 'hover:border-primary-main cursor-pointer border-gray-300' | ||
| }`} | ||
| onClick={handleClick} |
There was a problem hiding this comment.
There is quite a bit of inline CSS going on here. That makes the component hard to read. And probably also makes it tougher to change the styling later on. I think we should try to factor these things out of the render portion of the app and define such styles separately before the component, similar to how we define interfaces for the types. I think https://mui.com/system/styled/ should do the trick for that - e.g. like so https://mui.com/system/styled/#basic-usage
| dataTable: {}, | ||
| columns: {}, | ||
| uploadedDataDictionary: {}, | ||
| uploadedDataTableFileName: null, | ||
| uploadedDataDictionaryFileName: null, | ||
| setDataTable: (data: DataTable) => set({ dataTable: data }), |
There was a problem hiding this comment.
I find this store / section hard to read and think through. Couple ideas:
Is it really necessary to have dataTable, columns, uploadedDataTableFileName to all be separate store variables? I understand the benefit of avoiding deep updates, but all of these things are very closely releated, so representing them in a single object would make sense, no? If this is a big change, please make an issue to discuss instead.
We can make better use of whitespace and comments here to structure the sections and make them easier to understand. E.g. first section: // Data Table, followed by the variables, empty line, then all the setter and getter methods for dataTable stuff. Second section // Data dictionary, followed by same for dataDictionary.
This early in the process, it really pays of to think about cleanliness and structure. Once we start adding more things it'll get harder and harder to keep things clean and easy to understand
| UploadCard.defaultProps = { | ||
| diableFileUploader: false, | ||
| FileUploaderToolTipContent: 'Uploading is disabled', | ||
| }; | ||
|
|
There was a problem hiding this comment.
Is there a reason you use this format? If not, let's go with https://react.dev/learn/passing-props-to-a-component#specifying-a-default-value-for-a-prop. This seems to be the recommended approach for functional components and I think it also helps a lot with readability.
If there is a reason to keep using this format, please add a comment above the section to explain why, and then I'd say move the defaultProps section above the component declaration - because we'll most likely read this file top to bottom.
| FileUploader.defaultProps = { | ||
| disabled: false, | ||
| tooltipContent: 'Uploading is disabled', | ||
| }; | ||
|
|
There was a problem hiding this comment.
Same comment here as in UploadCard about the defaults. https://react.dev/learn/passing-props-to-a-component#specifying-a-default-value-for-a-prop or please comment why not like that and move above component for readability
Checklist
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
Implements a data dictionary upload feature, allowing users to upload a JSON file to provide metadata descriptions for data table columns. The file uploader component is enhanced with disabled state and tooltip support. This pull request closes issues #39 and #41.
New Features:
Enhancements:
Build:
ramdaand@microlink/react-json-viewas project dependencies.Chores: