-
Notifications
You must be signed in to change notification settings - Fork 657
CONSOLE-4695: Fixed SNC errors in /components/ lightspeed | modals | nav | file-upload #15413
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
base: main
Are you sure you want to change the base?
Conversation
/label px-approved |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: krishagarwal278 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@krishagarwal278: This pull request references CONSOLE-4695 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
/jira refresh |
@krishagarwal278: This pull request references CONSOLE-4695 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
/retest |
/cc |
@@ -13,8 +13,8 @@ export type FileUploadContextType = { | |||
setFileUpload: (file: File) => void; | |||
}; | |||
|
|||
export const FileUploadContext = createContext<FileUploadContextType>({ | |||
fileUpload: undefined, | |||
export const FileUploadContext = React.createContext<FileUploadContextType>({ |
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.
the namespace import returned as a result of merge conflict resolution
export const FileUploadContext = React.createContext<FileUploadContextType>({ | |
export const FileUploadContext = createContext<FileUploadContextType>({ |
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.
Yes, will remove namespace imports throughout this and other PRs
export const FileUploadContext = createContext<FileUploadContextType>({ | ||
fileUpload: undefined, | ||
export const FileUploadContext = React.createContext<FileUploadContextType>({ | ||
fileUpload: undefined as 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.
can we update FileUploadContextType
to be File | undefined
instead?
@@ -26,8 +26,8 @@ export const useValuesFileUploadContext = (): FileUploadContextType => { | |||
const [fileUploadExtensions, resolved] = useResolvedExtensions<FileUpload>(isFileUpload); | |||
const toastContext = useToast(); | |||
const [namespace] = useActiveNamespace(); | |||
const [file, setFile] = useState<File>(undefined); | |||
const fileExtensions = useMemo( | |||
const [file, setFile] = React.useState<File>(undefined as 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.
i believe asserting types as any
is an antipattern (as it inhibits type checking) and should be avoided in new/future code
const [file, setFile] = React.useState<File>(undefined as any); | |
const [file, setFile] = useState<File | undefined>(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.
Agreed. It'll defeat the purpose of type checking. I'll revert instances of any wherever i notice it
close(); | ||
history.push(resourceObjPath(cloneResource, referenceFor(cloneResource))); | ||
close?.(); | ||
history.push(resourceObjPath(cloneResource, referenceFor(cloneResource)) as 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.
same comment about usage of as any
@@ -262,7 +264,7 @@ const ClonePVCModal = withHandlePromise((props: ClonePVCModalProps) => { | |||
submitDisabled={!validSize || !pvcSC} | |||
errorMessage={errorMessage} | |||
submitText={t('console-app~Clone')} | |||
cancel={cancel} | |||
cancel={cancel as 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.
same comment about usage of as any
appendTo: () => | ||
document.querySelector("[data-test-id='perspective-switcher-toggle']") as HTMLElement, |
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.
does this work?
appendTo: () => | |
document.querySelector("[data-test-id='perspective-switcher-toggle']") as HTMLElement, | |
appendTo: () => | |
document.querySelector<HTMLElement>("[data-test-id='perspective-switcher-toggle']"), |
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.
Yeah it removed the type error earlier. We can also use generic type instead
@@ -151,7 +152,7 @@ const PinnedResource: React.FC<PinnedResourceProps> = ({ | |||
'oc-pinned-resource--dragging': draggable && isOver, | |||
})} | |||
> | |||
{draggable ? <DraggableButton dragRef={drag} /> : null} | |||
{draggable ? <DraggableButton dragRef={drag as any} /> : null} |
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.
insert as any
comment here
@@ -268,7 +268,7 @@ export type ModalSubmitFooterProps = { | |||
message?: string; | |||
errorMessage?: string; | |||
inProgress: boolean; | |||
cancel: (e: React.SyntheticEvent<any, Event>) => void; | |||
cancel?: (e: React.SyntheticEvent<any, Event>) => void; |
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.
given how we're trying to deprecate and rewrite these modals I don't think we should change the proptype if possible
Local tsc check passed against the files in pr code and regression test passed on cluster launched against the pr. |
@krishagarwal278: This pull request references CONSOLE-4695 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
/retest |
1 similar comment
/retest |
appendTo: () => { | ||
const element = document.querySelector<HTMLElement>( | ||
"[data-test-id='perspective-switcher-toggle']", | ||
); | ||
return element || document.body; | ||
}, |
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.
does e2e still work with this change?
appendTo: () => { | |
const element = document.querySelector<HTMLElement>( | |
"[data-test-id='perspective-switcher-toggle']", | |
); | |
return element || document.body; | |
}, | |
appendTo: "inline" |
/label tide/merge-method-squash |
…/src/components/ lightspeed | modals | nav | file-upload} modified: frontend/packages/console-app/src/components/file-upload/__tests__/file-upload-utils.spec.ts modified: frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx modified: frontend/packages/console-app/src/components/modals/resource-limits/ResourceLimitsModalLauncher.tsx modified: frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx modified: frontend/packages/console-app/src/components/nav/NavHeader.tsx modified: frontend/packages/console-app/src/components/nav/PinnedResource.tsx modified: frontend/packages/console-app/src/components/nav/PluginNavItem.tsx modified: frontend/packages/console-app/src/components/nav/utils.ts modified: frontend/public/components/factory/modal.tsx
318c354
to
bce3f4e
Compare
bce3f4e
to
3040319
Compare
@krishagarwal278: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Fixed SNC related errors in
/console-app/src/components/{packages/console-ap/src/components/ lightspeed | modals | nav | file-upload}