Skip to content
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

feat(ui-debug): add download in original file format #2277

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

hdinia
Copy link
Member

@hdinia hdinia commented Dec 19, 2024

No description provided.

@hdinia hdinia requested a review from skamril December 19, 2024 14:10
@hdinia hdinia self-assigned this Dec 19, 2024
@hdinia hdinia force-pushed the feature/add-download-in-original-format branch from 9aca50f to 49db115 Compare December 19, 2024 14:29
webapp/src/services/api/studies/raw/index.ts Outdated Show resolved Hide resolved
* @param params - Parameters for reading the matrix
* @param params.studyId - Unique identifier of the study
* @param params.path - Path to the matrix file
* @param params.format - Optional export format for the matrix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write @param [params.xxx] - ... instead of writting "Optional"

For format, header and index

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bracket notation is outdated and provides no value here since we rely on TypeScript interfaces to define optional parameters. Modern TypeScript/JSDoc doesn't use this syntax and you should only refer to Typescript to get this info

webapp/src/services/api/studies/raw/index.ts Outdated Show resolved Hide resolved
/**
* Uploads a file to a study's raw storage, creating or updating it based on existence.
*
* !Note: This method currently uses a poorly named endpoint (/raw). The endpoint structure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is more appropriate in the Python code or a ticket

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the API's current structure is leading to problematic usage patterns in the frontend, so it's actually valuable to document this in the client code. but the kind of detailed API design discussion should be in a dedicated ticket yeah

webapp/src/services/api/studies/raw/index.ts Outdated Show resolved Hide resolved
setCurrentJson(fileRes.data);
}, [fileRes.data]);

const handleDownload = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep it in the comment block

const handleDownload = async () => {
  const file = await getRawFile(studyId, filePath);
  downloadFile(file, filename);
}

Copy link
Member Author

@hdinia hdinia Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok didn't know the DownloadButton handles errors thanks it's better

@@ -24,23 +24,39 @@ import { downloadFile } from "../../../../../../utils/fileUtils";
import { useEffect, useState } from "react";
import { Filename, Flex, Menubar } from "./styles";
import UploadFileButton from "../../../../../common/buttons/UploadFileButton";
import { getRawFile } from "@/services/api/studies/raw";

function Json({ filePath, filename, studyId, canEdit }: DataCompProps) {
const [t] = useTranslation();
const { enqueueSnackbar } = useSnackbar();
const [currentJson, setCurrentJson] = useState<JSONEditorProps["json"]>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this state now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about this, I will check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setCurrentJson still needed

@hdinia hdinia force-pushed the feature/add-download-in-original-format branch from 49db115 to 04e415e Compare December 19, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants