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

new front end changes for the download api #406

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

AnkurPrabhu
Copy link
Contributor

@AnkurPrabhu AnkurPrabhu commented Nov 12, 2023

@AnkurPrabhu
Copy link
Contributor Author

@derneuere new pr for the download api as i was not able to find how to remove the package.json file from pr also
can you tell me what did u mean by Please format the code with our configurations

@sickelap
Copy link
Contributor

@AnkurPrabhu make sure that all PR checks are passing. Also enable pre-commit checks if you haven't done so yet. You can do that by running npm run prepare, having done that when you committing checks will run. Also look into sonar report and fix issues where it make sense.

src/App.tsx Outdated
@@ -1,6 +1,6 @@
import type { ColorScheme } from "@mantine/core";
import { AppShell, ColorSchemeProvider, MantineProvider } from "@mantine/core";
import { Notifications } from "@mantine/notifications";
import { NotificationsProvider } from "@mantine/notifications";
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make the build to fail. Mantine v6 does not have NotificationsProvider anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it will make a new image and then push my pr will fix my issues ig

shared: val_shared,
target_user_id: target_user.id,
})
.then(() => {
let notificationMessage = i18n.t("toasts.unsharephoto", {
let notificationMessage = i18n.t<string>("toasts.unsharephoto", {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you must to specify the type, it should be i18n.t<string|TemplateStringsArray, TOptions, string>("...").
As it looks too verbose and does not provide any value I suggest to remove type from translation function completely (as it was before) and let the type be inferred.

@AnkurPrabhu
Copy link
Contributor Author

@AnkurPrabhu make sure that all PR checks are passing. Also enable pre-commit checks if you haven't done so yet. You can do that by running npm run prepare, having done that when you committing checks will run. Also look into sonar report and fix issues where it make sense.

@sickelap thing is my pr only had 2 issues which were of a variable being used in function call when i rebased it ,it showed me 37 errors which were not on even related to my code so i pushed them

@sickelap
Copy link
Contributor

could it be that you rebased of the wrong commit?

@AnkurPrabhu
Copy link
Contributor Author

I had the latest dev branch-
I synced my fork then did a git pull on the dev branch of my local and then rebased that dev branch on my download branch

@AnkurPrabhu
Copy link
Contributor Author

AnkurPrabhu commented Nov 12, 2023

these are the errors i am getting not related to my code
Screenshot 2023-11-12 at 4 09 29 PM

also we should implement the commit hooks like: -
if any commit hook fails dont rollback other commit hook changes exampel prettier and other formatters

@sickelap any other code changes ? or thats it ?

@sickelap
Copy link
Contributor

make sure PR checks are passing

@AnkurPrabhu
Copy link
Contributor Author

@sickelap

these are the errors i am getting not related to my code Screenshot 2023-11-12 at 4 09 29 PM

also we should implement the commit hooks like: - if any commit hook fails dont rollback other commit hook changes exampel prettier and other formatters

@sickelap any other code changes ? or thats it ?

Copy link
Contributor

@sickelap sickelap left a comment

Choose a reason for hiding this comment

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

I have made the suggestions for most if not all lint check errors

});
return Server.post("photos/download", {
image_hashes: image_hashes,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
image_hashes: image_hashes,
image_hashes,

const startDownloadProcess = () => {
showNotification({
message: "Download Starting ...",
title: i18n.t<string>("toasts.downloadstart"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
title: i18n.t<string>("toasts.downloadstart"),
title: i18n.t("toasts.downloadstart"),

// eslint-disable-next-line import/no-cycle
import { Server } from "../api_client/apiClient";
import { Server,serverAddress } from "../api_client/apiClient";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { Server,serverAddress } from "../api_client/apiClient";
import { Server, serverAddress } from "../api_client/apiClient";

else if (status === "FAILURE") {
showNotification({
message: "Download failed",
title: i18n.t<string>("toasts.downloaderror"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
title: i18n.t<string>("toasts.downloaderror"),
title: i18n.t("toasts.downloaderror"),

dispatch({ type: "SET_PHOTOS_SHARED" });
Server.post(`photosedit/share/`, {
image_hashes,
image_hashes: image_hashes,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
image_hashes: image_hashes,
image_hashes,

});
};
}

export function editPhoto(image_hash: string, photo_details: any) {
return function cb(dispatch: Dispatch<any>) {
return function (dispatch: Dispatch<any>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return function (dispatch: Dispatch<any>) {
return function cb(dispatch: Dispatch<any>) {

Comment on lines 686 to 687
message: i18n.t<string>("toasts.editphoto"),
title: i18n.t<string>("toasts.editphototitle"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
message: i18n.t<string>("toasts.editphoto"),
title: i18n.t<string>("toasts.editphototitle"),
message: i18n.t("toasts.editphoto"),
title: i18n.t("toasts.editphototitle"),

Comment on lines 694 to 695
dispatch({ type: "EDIT_PHOTO_REJECTED" });
console.error(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dispatch({ type: "EDIT_PHOTO_REJECTED" });
console.error(error);
dispatch({ type: "EDIT_PHOTO_REJECTED", payload: error });

@@ -253,7 +253,7 @@
"favorites": "Favorites",
"mypublicphotos": "My Public Photos",
"photos": "Photos",
"videos": "Videos",
"videos":"Videos",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"videos":"Videos",
"videos": "Videos",

@@ -351,7 +351,7 @@
"notimestamp": "Photos without Timestamps",
"hidden": "Hidden Photos",
"favorite": "Favorite Photos",
"videos": "Videos",
"videos" :"Videos",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"videos" :"Videos",
"videos": "Videos",

Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
30.1% 30.1% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@AnkurPrabhu
Copy link
Contributor Author

all the eslint issues are solved and i currently see 0 errors so should be good to go learned a lot of new things while solving these errors, @sickelap sorry to bother you but your suggestions did help a lot thank-you also did u manually point all of these issues or used some extension to do it ? (more than 56 suggestions i think) sorry for the trouble

@AnkurPrabhu
Copy link
Contributor Author

AnkurPrabhu commented Nov 12, 2023

@derneuere @sickelap pls let me know if there are any changes needed lets merge this asap I want to see this change live Im really excited for this one 🥂

@sickelap
Copy link
Contributor

all the eslint issues are solved and i currently see 0 errors so should be good to go learned a lot of new things while solving these errors, @sickelap sorry to bother you but your suggestions did help a lot thank-you also did u manually point all of these issues or used some extension to do it ? (more than 56 suggestions i think) sorry for the trouble

Not a problem. Suggestions is a feature of GitHub PR review.

@sickelap
Copy link
Contributor

Please update LibrePhotos/librephotos#1057 so all checks are green. After that's done I will verify the feature and both PRs can merged

@sickelap sickelap merged commit 6ae3035 into LibrePhotos:dev Nov 14, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants