Skip to content

Add learning by sector,regions bar chart and Map for learning #1577

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jan 29, 2025
Merged

Add learning by sector,regions bar chart and Map for learning #1577

merged 11 commits into from
Jan 29, 2025

Conversation

roshni73
Copy link
Collaborator

@roshni73 roshni73 commented Dec 5, 2024 •
edited by samshara
Loading

Addresses:

Changes

  • Add learning by sector, region, and source over time chart.
  • Add Map view for operational learning

Depends on

This PR doesn't introduce:

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

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 5, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 3123692

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@roshni73 roshni73 marked this pull request as draft December 5, 2024 10:48
@roshni73 roshni73 force-pushed the feature/add-charts-learnings branch from 89e0a01 to e80e0a5 Compare December 6, 2024 08:48
samshara
samshara requested changes Dec 9, 2024
View reviewed changes
@samshara samshara self-requested a review December 10, 2024 09:58
@roshni73 roshni73 force-pushed the feature/add-charts-learnings branch from e80e0a5 to 3bde6b9 Compare December 11, 2024 04:21
@roshni73 roshni73 force-pushed the feature/add-charts-learnings branch from 3bde6b9 to 3ef7309 Compare December 11, 2024 05:44
@roshni73 roshni73 force-pushed the feature/add-charts-learnings branch from 3ef7309 to a9f5ebb Compare December 11, 2024 05:55
@roshni73 roshni73 force-pushed the feature/add-charts-learnings branch from a9f5ebb to 3071d1b Compare December 11, 2024 06:39
@roshni73 roshni73 force-pushed the feature/add-charts-learnings branch 2 times, most recently from 6af5950 to 087c25b Compare December 12, 2024 05:38
@frozenhelium frozenhelium force-pushed the project/operational-learning-2.0 branch from 92f7f35 to b9a1d6a Compare January 29, 2025 06:54
@samshara samshara force-pushed the feature/add-charts-learnings branch from dc97d89 to 3123692 Compare January 29, 2025 07:54
@samshara samshara merged commit c1f350e into project/operational-learning-2.0 Jan 29, 2025
17 of 18 checks passed
@samshara samshara deleted the feature/add-charts-learnings branch January 29, 2025 07:59
tnagorra
tnagorra reviewed Jan 29, 2025
View reviewed changes
app/src/views/OperationalLearning/Stats/index.tsx
Comment on lines +142 to +143
const oldestDate = new Date(Math.min(...dates.map((date) => date.getTime())));
const latestDate = new Date(Math.max(...dates.map((date) => date.getTime())));
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 to check if length of days is zero.

tnagorra
tnagorra reviewed Jan 29, 2025
View reviewed changes
app/src/views/OperationalLearning/Stats/index.tsx
compactMessage
pending={learningStatsPending}
empty={isDefined(learningStatsResponse?.learning_by_sector) && (
(learningStatsResponse?.learning_by_sector.length ?? 0) < 1
Copy link
Member

Choose a reason for hiding this comment

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

No need to add default value if isDefined is already used.

tnagorra
tnagorra reviewed Jan 29, 2025
View reviewed changes
app/src/views/OperationalLearning/Stats/index.tsx
withInternalPadding
compactMessage
pending={learningStatsPending}
empty={isDefined(learningStatsResponse?.learning_by_region) && (
Copy link
Member

Choose a reason for hiding this comment

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

No need to add default value if isDefined is already used.

tnagorra
tnagorra reviewed Jan 29, 2025
View reviewed changes
app/src/views/OperationalLearning/Stats/index.tsx
pending={learningStatsPending}
childrenContainerClassName={styles.chartContainer}
empty={isDefined(learningStatsResponse?.sources_overtime) && (
(learningStatsResponse?.sources_overtime.length ?? 0) < 1
Copy link
Member

Choose a reason for hiding this comment

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

No need to add default value if isDefined is already used.

tnagorra
tnagorra reviewed Jan 29, 2025
View reviewed changes
app/src/views/OperationalLearning/Stats/index.tsx
strings.sourceOthers,
]);

const activePointData = activePointKey ? sourcesOverTimeData?.[activePointKey] : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Let's use isDefined.

tnagorra
tnagorra reviewed Jan 29, 2025
View reviewed changes
app/src/views/OperationalLearning/Stats/index.tsx
<div className={styles.legendLabel}>
{strings.sourcesTypeLegendLabel}
</div>
<div className={styles.legendContent}>
Copy link
Member

Choose a reason for hiding this comment

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

Do we mount the container if the list of items is zero?

tnagorra
tnagorra reviewed Jan 29, 2025
View reviewed changes
app/src/views/OperationalLearning/Stats/OperationalLearningMap/index.tsx
Comment on lines +88 to +89
const features = learningByCountry
?.map((value) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const features = learningByCountry
?.map((value) => {
const features = learningByCountry
.map((value) => {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnagorra tnagorra tnagorra left review comments

@AdityaKhatri AdityaKhatri AdityaKhatri approved these changes

@samshara samshara samshara approved these changes

@barshathakuri barshathakuri Awaiting requested review from barshathakuri

@frozenhelium frozenhelium Awaiting requested review from frozenhelium

@subinasr subinasr Awaiting requested review from subinasr

@shreeyash07 shreeyash07 Awaiting requested review from shreeyash07

Assignees

@AdityaKhatri AdityaKhatri

@samshara samshara

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@roshni73 @tnagorra @frozenhelium @AdityaKhatri @samshara @shreeyash07 @subinasr