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

Heatmap Reduced List #269

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

Heatmap Reduced List #269

wants to merge 20 commits into from

Conversation

shami-sneha
Copy link

@shami-sneha shami-sneha commented Jun 19, 2023

Description

language switch

Related Issues

#192

Design Decisions

To reduce heatmap list.

Performance & Quality

Checklist

I, the author of this PR checked the following requirements for good software quality:

  • The code is properly formatted (I ran the formatter)
  • The code is written with our software quality standards (I ran the linter)
  • The code is written using our code style
  • Extensive in source documentation has been added
  • Unit and/or integration tests have been added
  • All texts have been internationalized with at least the following languages:
    • English
    • German
  • I tried addressing all new accessibility problems displayed in the console and documented if they can't be fixed
  • I attached performance measurements to prevent performance degradation
  • I added the changes to the next release section of the changelog

I, the reviewer checked the following things:

  • I ran the software once and tried all new and related functionality to this PR
  • I looked at all new and changed lines of code and commented on possible problems
  • I read the added documentation and checked if it is understandable and clear
  • I checked the added tests for completeness
  • I checked the internationalized strings for spelling errors
  • I checked the performance metrics for problems or unexplained degradation
  • I checked that the changes are noted in the changelog

@shami-sneha shami-sneha changed the title Feature/filter heatmap Heatmap Reduced List Jun 19, 2023
@github-actions
Copy link

github-actions bot commented Jun 28, 2023

Unit Test Results

0 files   -   1  0 suites   - 14   0s ⏱️ -33s
0 tests  - 32  0 ✔️  - 32  0 💤 ±0  0 ±0 
0 runs   - 33  0 ✔️  - 33  0 💤 ±0  0 ±0 

Results for commit db786ea. ± Comparison against base commit 51024b2.

♻️ This comment has been updated with latest results.

@shami-sneha shami-sneha marked this pull request as ready for review July 11, 2023 09:52
@NXXR
Copy link
Collaborator

NXXR commented Jul 14, 2023

@shami-sneha the TS-compile error in CollectAttributions.ts was fixed in #280.
So make sure to merge the latest changes from develop into your branch and see if the pipelines go through.

Copy link
Collaborator

@NXXR NXXR left a comment

Choose a reason for hiding this comment

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

Seems to be working fine when I apply the suggestion mentioned up top to wrap the presets file.

Suggestions to address:

  • When selecting a heat legend preset in the expanded list (show more = true) that is not in the reduced list,
    the selection shows an empty value when going back to the reduced list (show more = false).
    Maybe we can add the currently selected heat legend also to the the reduced list (if not present) to avoid showing an empty value in the drop down menu.

Comment on lines 37 to 46
'simulation-start-day': 'Simulationsstart',
add_button: 'Fächer',
},
'group-filters': {
title: 'Gruppen Verwalten',
'title-manage-compartment': 'Fächer verwalten',
'nothing-selected': 'Wählen Sie eine Gruppe aus um diese zu bearbeiten oder erstellen Sie eine neue Gruppe.',
'add-group': 'Neue Gruppe Erstellen',
'add-group-compartment': 'Fächer',
name: 'Name',
Copy link
Collaborator

Choose a reason for hiding this comment

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

These seem to belong to a different PR (#58)

Suggested change
'simulation-start-day': 'Simulationsstart',
add_button: 'Fächer',
},
'group-filters': {
title: 'Gruppen Verwalten',
'title-manage-compartment': 'Fächer verwalten',
'nothing-selected': 'Wählen Sie eine Gruppe aus um diese zu bearbeiten oder erstellen Sie eine neue Gruppe.',
'add-group': 'Neue Gruppe Erstellen',
'add-group-compartment': 'Fächer',
name: 'Name',
'simulation-start-day': 'Simulationsstart',
},
'group-filters': {
title: 'Gruppen Verwalten',
'nothing-selected': 'Wählen Sie eine Gruppe aus um diese zu bearbeiten oder erstellen Sie eine neue Gruppe.',
'add-group': 'Neue Gruppe Erstellen',
name: 'Name',

Comment on lines 63 to 66

haetmapchart: {
textshow: 'Zeig mehr',
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a typo here preventing the german translation to be used

Suggested change
haetmapchart: {
textshow: 'Zeig mehr',
},
heatmapchart: {
textshow: 'Mehr anzeigen',
},

Comment on lines 70 to 72
heatmapchart: {
textshow: 'show more',
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

All texts should start with a capital letter unless there is a good reason for it not to.

Suggested change
heatmapchart: {
textshow: 'show more',
},
heatmapchart: {
textshow: 'Show more',
},

@@ -11,6 +11,7 @@
accessibility: 'Accessibility',
attribution: 'Attribution',
changelog: 'Changelog',
'changelog-path': 'locales/en/changelog-en.md',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a leftover from a merge conflict. PR#294 removed the changelog-path property.

Suggested change
'changelog-path': 'locales/en/changelog-en.md',

@@ -99,6 +99,7 @@ export default function DistrictMap(): JSX.Element {

// fetch geojson
useEffect(() => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this added?
since it's missing in develop it should run fine without?

if (heatmapLegends.length === 0 && heatLegendEditOpen) {
fetch(legendPresets, {
if (legend.name == 'uninitialized') {
fetch('assets/heatmap_legend_presets.json', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have to warp assets for the to work with the changes introduced in PR#249.
Refer to the PR description for further info.

Suggested change
fetch('assets/heatmap_legend_presets.json', {
fetch('legendPresets', {

import {HeatmapLegend} from '../../types/heatmapLegend';
import {useTranslation} from 'react-i18next';
import legendPresets from 'assets/heatmap_legend_presets.json';
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment on line 50 and refer to PR#249 for further info.

Suggested change
import legendPresets from 'assets/heatmap_legend_presets.json';
import legendPresets from 'assets/heatmap_legend_presets.json';

Comment on lines +92 to +93
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

PLease add comments to the dependency list of effects to show when & why the effect should run:

Suggested change
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
// This effect should only run once on first use since the presets don't change.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

Comment on lines 95 to 99
useEffect(() => {
if (!activeScenario) {
return;
if (activeScenario) {
dispatch(selectDefaultLegend({selectedScenario: activeScenario - 1}));
}

if (legend.name !== 'Default' && legend.name !== 'uninitialized') {
return;
}

selectLegendByName('Default');
}, [activeScenario, legend, selectLegendByName]);
}, [activeScenario, dispatch]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add comments to this effect:

Suggested change
useEffect(() => {
if (!activeScenario) {
return;
if (activeScenario) {
dispatch(selectDefaultLegend({selectedScenario: activeScenario - 1}));
}
if (legend.name !== 'Default' && legend.name !== 'uninitialized') {
return;
}
selectLegendByName('Default');
}, [activeScenario, legend, selectLegendByName]);
}, [activeScenario, dispatch]);
// Effect to update the default legend when the active scenario changes
useEffect(() => {
if (activeScenario) {
// Select default legend (legends are in a 0-based array while the scenario Ids are 1-based)
dispatch(selectDefaultLegend({selectedScenario: activeScenario - 1}));
}
// This effect should run whenever the active scenrio changes (dispatch should not change during runtime).
}, [activeScenario, dispatch]);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add comments to your code to make it easier to grasp at a glancec what is happening.
This helps other contributors when they need to handle parts of your code.

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