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

feat: initial indicator dashboards [WAC-85] #1

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

jonathansberry
Copy link
Member

@jonathansberry jonathansberry commented Jul 1, 2024

Description

@Manoj-nathwani's work creating three dashboard components for indicators:

  • bar chart
  • line chart
  • world map

Dependency Changes

  • Added a PipFile and lock for the proxy server
  • Updated the package.json and lock with the new repo name

Testing

No tests have been made.

Documentation

  • New root README.md describes the overall project, how to use it in dev and build
  • No documentation of the components themselves.

Internationalisation and Localisation

None done so far.

Checklist

  • The Jira ticket for this issue has been updated to "Ready to Review" or equivalent.
  • I have developed these changes in discussion with the appropriate project manager.
  • My code follows the general Fjelltopp documentation (see Confluence).
  • I have made corresponding changes to the Fjelltopp documentation (see Confluence).
  • I have rebased this branch with master.
  • New dependency changes have been committed.
  • I have added automated tests that prove my fix is effective or that my feature works.
  • New and existing tests pass locally with my changes.
  • My changes generate no new warnings.
  • I have performed a self-review of my own code.
  • I have assigned at least one reviewer.
  • I have assigned at least one label to this PR: "patch", "minor", "major".

@ChasNelson1990 ChasNelson1990 changed the title Indicator data dashboard JS feat: initial indicator dashboards [WAC-85] Jul 1, 2024
@ChasNelson1990 ChasNelson1990 added the enhancement New feature or request label Jul 1, 2024
Copy link
Member

@ChasNelson1990 ChasNelson1990 left a comment

Choose a reason for hiding this comment

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

Are we missing the BarChart as per the ToR?

I am not sure that colours should be being passed in via the HTML data props? I think that we should be using the WHO colour palette and defining it in the CSS or JS and the charts should cycle through those colours.

Finally, you'll see I've added eslint and prettier; I have done a prettier format for you but if you run npm run lint you'll see some errors that need fixing.

@@ -0,0 +1 @@
v20.14.0
Copy link
Member

Choose a reason for hiding this comment

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

Why v20.14.0, my NVM is telling me that LTS Iron should be v20.15.0? Has that just been released since you did this work?

@@ -0,0 +1,33 @@
<img width="1027" alt="image" src="https://github.com/Manoj-nathwani/WHO-African-Regional-Health-Data-Hub-React-Components/assets/2634482/98cb7219-a767-4705-ab2d-2e7f0a38e220">
Copy link
Member

Choose a reason for hiding this comment

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

This is pointing to the old repo and needs to be updated.


This repository contains small, independent React components designed to be integrated into CKAN resource templates. These components can be individually built and added to Jinja2 templates for CKAN resources.

The project uses [parcel](https://parceljs.org) to build components into their respective directories. This approach allows for the creation of small, independent components that can be reused and repurposed while maintaining a consistent development workflow.
Copy link
Member

Choose a reason for hiding this comment

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

A list of component names and how to import them into other projects probably belongs here.

Copy link
Member

Choose a reason for hiding this comment

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

This top-level README is great, thanks @Manoj-nathwani. But I think we should also have component-level READMEs in the ckan and map folders detailing the components and their props.

"src/map/index.html",
"src/ckan/index.html"
],
"browserslist": "> 0.5%, last 2 versions, not dead",
Copy link
Member

Choose a reason for hiding this comment

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

Why not defaults?

}
);

export default ConfigContext
Copy link
Member

Choose a reason for hiding this comment

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

Just a style preference here but can we use named exports, i.e. add export to the start of line 4 and remove the default. Same elsewhere


const [selectedCountry, setSelectedCountry] = useState<selectedCountryType>();
const [selectedYear, setSelectedYear] = useState<selectedYearType>();
const [selectedSex, setSelectedSex] = useState<selectedSexType>(config.SEXES[0].id);
Copy link
Member

Choose a reason for hiding this comment

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

Is this always sex? Perhaps it should be called "facet" or similar?

</head>
<body>

<!-- <div
Copy link
Member

Choose a reason for hiding this comment

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

Why a big block of commented out HTML?


export default function Map({ country, setCountry }) {

const colorScale = scaleQuantize()
Copy link
Member

Choose a reason for hiding this comment

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

Is this used?

setCountry(geo)
}
}}
// fill={colorScale(cur ? cur.unemployment_rate : "#EEE")}
Copy link
Member

Choose a reason for hiding this comment

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

?

@ChasNelson1990
Copy link
Member

Are we missing the BarChart as per the ToR?

Discussed this with @jonathansberry and we are happy with the table-based solution to this produce this. However, we need to bound it in a sensible vertical scroll-box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants