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

Features/geo area rest graphql #2962

Merged
merged 5 commits into from
Aug 16, 2024
Merged

Conversation

shreeyash07
Copy link
Contributor

@shreeyash07 shreeyash07 commented Jun 19, 2024

Changes

  • Rest to Graqhql for
    • Create Region
    • Create Admin Level
    • Update Admin Level
    • Publish Region
    • Delete Region
    • Fetch Admin level
  • Replace file upload with rest to graphql

This PR doesn't introduce any:

  • temporary files, auto-generated files or secret keys
  • build works
  • eslint issues
  • typescript issues
  • codegen errors
  • console.log meant for debugging
  • typos
  • unwanted comments
  • conflict markers

This PR contains valid:

  • permission checks
  • translations

@shreeyash07 shreeyash07 force-pushed the features/geo-area-rest-graphql branch 3 times, most recently from 8b72bb3 to 4c1b89e Compare June 25, 2024 09:55
@@ -50,6 +53,10 @@ function FileUpload(props: Props) {
disabled,
projectIds,
buttonOnly,
// value: valueFromProps,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unwanted comments.

onChange={handleFileInputChange}
status={undefined}
status={status ? currentStatus : ''}
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
status={status ? currentStatus : ''}
status={status ? currentStatus : undefined}

@@ -122,16 +132,31 @@ function FileUpload(props: Props) {
],
);

const currentStatus = useMemo(() => {
if (isNotDefined(status)) {
return '';
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 '';
return undefined;

@shreeyash07 shreeyash07 marked this pull request as ready for review June 27, 2024 04:55
@shreeyash07 shreeyash07 requested a review from barshathakuri July 3, 2024 04:23
@subinasr subinasr force-pushed the features/geo-area-rest-graphql branch from 82804b6 to 9106c9f Compare July 16, 2024 04:04
@@ -69,6 +72,7 @@ function RegionSelectInput<K extends string, GK extends string>(
const variables = useMemo(() => ({
excludeProject: [projectId],
search: debouncedSearchText,
public: true,
Copy link
Member

Choose a reason for hiding this comment

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

Nice. Much awaited 😄

Comment on lines 32 to 41
createRegion(
data: $data
) {
ok
errors
result {
id
isPublished
}

}
Copy link
Member

Choose a reason for hiding this comment

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

fix indentation

Comment on lines 47 to 69
ok
errors
result {
tolerance
title
staleGeoAreas
parentNameProp
parentCodeProp
codeProp
nameProp
geoShapeFile {
file {
url
}
id
title
mimeType
}
level
id
}
Copy link
Member

Choose a reason for hiding this comment

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

fix indent

Comment on lines 156 to 160
const initialFormValue: PartialAdminLevelInput = {
...remainingValues,
geoShapeFile: geoShapeFile?.id,
region: regionId,
};
Copy link
Member

Choose a reason for hiding this comment

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

use useMemo


const {
pending: deleteAdminLevelPending,
trigger: deleteAdminLevel,
} = useLazyRequest<unknown, number>({
} = useLazyRequest<unknown, string>({
Copy link
Member

Choose a reason for hiding this comment

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

need to replace this function as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Backend work is left.

Comment on lines 209 to 212
const valueWithClientId = {
...value,
clientId: value.id.toString(),
};
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we don't need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i checked it, we don't need it anymore

patchRegion({
variables: {
projectId,
regionId: [String(value)],
Copy link
Member

Choose a reason for hiding this comment

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

Won't this replace the previous regions and only set one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mutation only requires selected region for the patch not the entire list.

Copy link
Member

Choose a reason for hiding this comment

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

Okayy, thenks

@shreeyash07 shreeyash07 requested a review from AdityaKhatri July 26, 2024 04:12
@shreeyash07 shreeyash07 force-pushed the features/geo-area-rest-graphql branch 3 times, most recently from 5a2006d to 2bada9c Compare July 30, 2024 04:32
import { FileInput, useAlert } from '@the-deep/deep-ui';
import { IoCloudUpload } from 'react-icons/io5';
import { gql, useMutation } from '@apollo/client';
import { removeNull } from '@togglecorp/toggle-form';
import { _cs } from '@togglecorp/fujs';
import { _cs, isDefined, isNotDefined } from '@togglecorp/fujs';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's separate these out in multiple lines.


if (isDefined(result) && ok) {
alert.show(
'Admin level is successfully update!',
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
'Admin level is successfully update!',
'Admin level is successfully updated!',

);
submit();
}, [setError, validate, addAdminLevelTrigger]);
}, [setError, validate, updateAdminLevel, createAdminLevel, valueFromProps.id]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's separate these in multiple lines.

@@ -1,14 +1,14 @@
import React, { useCallback, useMemo, useState } from 'react';
import {
_cs,
randomString,
_cs, isNotDefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate lines please.

{
skip: !isExpanded,
variables: adminLevelQueryVariables,
// NOTE: this prop is required for onCompleted to be
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!


if (errors) {
alert.show(
'Failed to publish selected region.',
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
'Failed to publish selected region.',
'Failed to delete selected region.',

@shreeyash07 shreeyash07 force-pushed the features/geo-area-rest-graphql branch from 2bada9c to a2107df Compare July 30, 2024 12:03
@shreeyash07 shreeyash07 requested a review from subinasr July 30, 2024 12:05
@AdityaKhatri AdityaKhatri force-pushed the features/geo-area-rest-graphql branch from a2107df to 7908dd9 Compare August 12, 2024 08:26
@AdityaKhatri AdityaKhatri merged commit 41d87a6 into develop Aug 16, 2024
4 of 5 checks passed
@AdityaKhatri AdityaKhatri deleted the features/geo-area-rest-graphql branch August 16, 2024 06:24
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.

4 participants