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

Add/analysis overview query #2970

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

barshathakuri
Copy link
Contributor

@barshathakuri barshathakuri commented Jul 18, 2024

Addresses

Depends on: the-deep/server#1516

Changes

  • Fetch analysis overview client
  • Fetch analysis summer client
  • Fetch analysis detail client
  • Fetch pillar summary

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

@barshathakuri barshathakuri marked this pull request as draft July 18, 2024 10:02
@barshathakuri barshathakuri force-pushed the add/analysis-overview-query branch 3 times, most recently from 84c8175 to 9324507 Compare July 18, 2024 10:18
} = useForm(schema, defaultFormValue);

const error = getErrorObject(riskyError);

/*
Copy link
Member

Choose a reason for hiding this comment

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

yo block nai hatauda huncha

const onDeleteConfirmClick = useCallback(() => {
onDelete(pillarId);
onDelete(Number(pillarId));
Copy link
Member

Choose a reason for hiding this comment

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

Leave a TODO here as well

@@ -74,13 +71,11 @@ function AnalysisPillar(props: Props) {
statusLabel = _ts('analysis', 'noAnalysisTagLabel');
}

const disabled = pendingPillarDelete;
Copy link
Member

Choose a reason for hiding this comment

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

We need this, don't we?

projectId: activeProject,
title: data.title,
pendingPillarDelete: pendingPillarDelete && data.id === deletePillarId,
Copy link
Member

Choose a reason for hiding this comment

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

Are we moving this inside?

@@ -429,7 +427,7 @@ function Analysis(props: ComponentProps) {
<ExpandableContainer
className={styles.pillarAnalyses}
headerClassName={styles.pillarAnalysesHeader}
heading={_ts('analysis', 'pillarAnalysisCount', { count: pillars.length })}
heading={_ts('analysis', 'pillarAnalysisCount', { count: pillars?.length })}
Copy link
Member

Choose a reason for hiding this comment

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

Lets remove _ts and replace this directly with a text.

Comment on lines 200 to 202
const timelineLabelSelector = (d: TimelineData): string => d.label;
const timelineValueSelector = (d: TimelineData): number => d.value;
const timelineKeySelector = (d: TimelineData): string => d.key;
Copy link
Member

Choose a reason for hiding this comment

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

why replace the d?

const totalLeadsCount = analysisSummaryData
?.project?.analysisOverview?.totalLeadsCount;

const analysisRendererParams = useCallback((id: string, data: AnalysisSummaryType) => ({
Copy link
Member

Choose a reason for hiding this comment

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

Add return time in renderer params for better typecheck

@barshathakuri barshathakuri marked this pull request as ready for review August 28, 2024 08:46

const handleAnalysisEditClick = useCallback((analysisId: number | undefined) => {
Copy link
Member

Choose a reason for hiding this comment

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

Please do not undo this

interface AnalysisDashboardProps {
className?: string;
analysisId: string;
Copy link
Member

Choose a reason for hiding this comment

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

why? Please no

@AdityaKhatri AdityaKhatri force-pushed the add/analysis-overview-query branch 2 times, most recently from 6866c77 to 7ab476d Compare September 20, 2024 04:39
@AdityaKhatri AdityaKhatri force-pushed the add/analysis-overview-query branch 4 times, most recently from 81ee1a1 to 4f874bf Compare October 18, 2024 11:01
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