-
Notifications
You must be signed in to change notification settings - Fork 39
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
[MDS-5656] Add Report Details - CRR #2851
Conversation
onAfterResponse: () => {}, | ||
}; | ||
|
||
class FileUpload extends React.Component { |
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.
Interesting. I really like the idea of having the fileUpload component moved to the common package, but I don't think they behave in exactly the same way.
We don't need to remove this, but we'll need to consider rendering the differences seperately for each FE if we want to use this in Core.
@@ -0,0 +1,76 @@ | |||
import React from "react"; |
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.
Hmmm, this might get a bit sticky. Tara has spent a bunch of time moving all of the form components into the common package and converting them to .tsx. Going to have to coordinate merging these I think.
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.
Removed unnecessary props.
@@ -0,0 +1,137 @@ | |||
import { ENVIRONMENT } from "@mds/common"; |
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.
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.
Removed.
@@ -0,0 +1,45 @@ | |||
// import AddReportModal from "@/components/modalContent/reports/AddReportModal"; | |||
import EditReportModal from "@mds/common/components/modalContent/reports/EditReportModal"; | |||
// import AddVarianceModal from "@/components/modalContent/variances/AddVarianceModal"; |
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 like having a shared config for this, but I think we just delete everything not being used, and add them back in as needed. (Honestly would rather get rid of the config altogether, but I don't think that's happening right away)
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.
Removed as these became unnecessary after addressing all the comments.
|
||
interface AddReportDetailsProps { | ||
mineGuid: string; | ||
updateMineReportSubmissions: any; |
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.
For anything coming from the mapDispatchToProps
we can add a type like this:
updateMineReportSubmissions: ActionCreator<typeof updateMineReportSubmissions>;
This will give you all the correct typing when working with it throughout the file and make it easier to maintain.
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.
Updated.
|
||
const AddReportDetails: FC<AddReportDetailsProps> = (props) => { | ||
const { mineGuid, dropdownMineReportCategoryOptions } = props; | ||
const [state, setState] = 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.
Is this just an artifact of coming from a class based component at one point in time?
I think it's much cleaner to seperate all states into their own useStates as you've done lower down. Makes it more consistent and readable. And pretty. It also makes it pretty.
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.
Updated.
@@ -0,0 +1,602 @@ | |||
import { |
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.
This also exists in
services/common/src/redux/utils/helpers.ts
💃🏻 🕺🏻
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.
Removed
|
||
<Col span={24}> | ||
<Form layout="vertical"> | ||
<ReportsTable |
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 think this might not be the correct table. I think the designs are just asking for a table of the documents, not the other reports.
I see you brought over the editReport modal and a few other things that are related to this table, so it would probably mean that we can leave those back where they were.
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.
As discussed added a new component.
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.
Lots of awesome work here! I made a few comments, but the big one I think will need to be dealt with is the table at the bottom of your new page should just be a document table instead of the reports table.
Once that is swapped out, I think you'll find that a fair amount of the stuff moved to common doesn't need to be there (I think the reports table itself is fairly Minespace specific)
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.
Noooooo we don't need more copies of this file.
Use the antd one for actions menu.
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 might be wrong but it doesn't seem like you are using this image in your changes? Might be able to just remove this file.
@@ -0,0 +1,280 @@ | |||
/* eslint-disable */ | |||
import React from "react"; |
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.
now that I merged my PR, I think it's just the Callout and LinkButton that aren't already in common. This one's called RenderFileUpload now.
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.
This is updated accordingly
@@ -0,0 +1,19 @@ | |||
import { PropTypes, shape, arrayOf, oneOfType } from "prop-types"; |
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'm not sure about bringing in customPropTypes to common when we're phasing it out. What can we do to remove it instead?
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.
This is removed.
Generally looks good, but I do want to avoid pulling a bunch of stuff into common that's already slated for deletion, most notably customPropTypes which we use interfaces for now instead. |
@@ -0,0 +1,20 @@ | |||
// Redux Forms | |||
export const ADD_REPORT = "ADD_REPORT"; |
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 think I also remade forms.js as a forms enum. Still works pretty much the same way but it only has one entry so far and doesn't trigger duplicate named import warnings/errors.
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.
Nice! Thanks for all the hard work on this one. Sorry the scope got a little out of control!
Objective
MDS-5656