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.

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

Add color in the per dashboard and per export #1361

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

Add color in the per dashboard and per export #1361

wants to merge 3 commits into from

Conversation

barshathakuri
Copy link
Collaborator

@barshathakuri barshathakuri commented Sep 11, 2024 •
edited
Loading

Addresses:

https://github.com/orgs/IFRCGo/projects/11/views/1filterQuery=is%3Aopen++assignee%3A%40me&pane=issue&itemId=62290081

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 Sep 11, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 712f3ff

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

@barshathakuri barshathakuri marked this pull request as draft September 11, 2024 11:16
@barshathakuri barshathakuri marked this pull request as ready for review September 12, 2024 10:34
tnagorra
tnagorra requested changes Sep 13, 2024
View reviewed changes
app/src/views/CountryPreparedness/PreviousAssessmentChart/styles.module.css Outdated
@@ -2,7 +2,7 @@
height: 20rem;

.path {
stroke: var(--go-ui-color-primary-red);
stroke: #236192;
Copy link
Member

Choose a reason for hiding this comment

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

@frozenhelium shouldn't we use a design token for this as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed; it was missed.

app/src/views/CountryPreparedness/PrivateCountryPreparedness/index.tsx Outdated
</div>
),
(priorityComponent) => {
const progressBarColor = priorityComponent.num !== 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 function instead.

app/src/views/CountryPreparedness/PrivateCountryPreparedness/index.tsx Outdated
Comment on lines 659 to 673
const area = assessmentStats?.topRatedComponents.find(
(component) => component.area.area_num === Number(areaNum),
);

if (!area) {
return null;
}
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 create a dictionary for top rated components instead of using find inside a map

app/src/views/CountryPreparedness/PrivateCountryPreparedness/index.tsx Outdated
Comment on lines 671 to 675
`
${strings.areaLegend}
${areaNum}
${getFormattedComponentName(area.area)}
`
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to break these down to new lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It exceeds more than 100 so. I will fix it.

app/src/views/PerExport/index.tsx Outdated
Comment on lines 553 to 567
const area = assessmentStats?.topRatedComponents.find(
(component) => component.area.area_num === Number(areaNum),
);

if (!area) {
return null;
}
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 not use find inside map

app/src/views/PerExport/index.tsx Outdated
Comment on lines 565 to 568
`
${strings.areaLegend}
${areaNum}
${getFormattedComponentName(area.area)}
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to break into multiple lines?

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

@tnagorra tnagorra tnagorra requested changes

@frozenhelium frozenhelium Awaiting requested review from frozenhelium

@roshni73 roshni73 Awaiting requested review from roshni73

@samshara samshara Awaiting requested review from samshara

@shreeyash07 shreeyash07 Awaiting requested review from shreeyash07

@puranban puranban Awaiting requested review from puranban

Requested changes must be addressed to merge this pull request.

Assignees

@samshara samshara

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@barshathakuri @tnagorra @samshara