search

ooni / explorer

OONI Explorer: uncover evidence of internet censorship worldwide
https://explorer.ooni.org
BSD 3-Clause "New" or "Revised" License
71 stars 37 forks source link

replaced older IM and circumvention charts #785

Closed GermaVinsmoke closed 1 year ago

GermaVinsmoke commented 2 years ago

Signed-off-by: GermaVinsmoke vaibhav1180@gmail.com

vercel[bot] commented 2 years ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
explorer ✅ Ready (Inspect) Visit Preview Dec 29, 2022 at 2:56PM (UTC)
GermaVinsmoke commented 2 years ago

Had to add a separate entry point Chart.js because the way query and query params are formed are different in the Chart.js of network page. I think if we can separate out the query generation logic and useSwr query hook into separate files then we can generalise the Chart.js file into a single component.

hellais commented 2 years ago

This looks great, thanks for putting it together. Some things I think we should do before merging it are:

  1. Remove the commented out parts of code and delete them
  2. There are several components that are no longer needed and they should also be removed (e.g. inCountry context)
  3. We need some sort of time interval picker for the charts on the country page (similar to what we have on the network page) so that the user is able to filter by a specified date range
  4. Some bits of the page are not translatable (i.e. they need to be wrapped using the FormattedMessage component), I will add an inline comment for these
  5. Since the country and network page charts are different, to avoid confusion I would give these two components a different name (e.g. ChartCountry, ChartNetwork) and eventually consider merging them as you said in your comment.

Next steps, which can be done as part of a separate PR:

  1. Refactor the coverage chart to make use of the coverage chart we have in the network page (while you are at it I would also address: https://github.com/ooni/explorer/issues/771)
  2. As you mentioned in your above comments, look into the possibility of sharing the Chart component between the country and network pages

Other nice to have improvements to the country page include:

GermaVinsmoke commented 2 years ago

Hey @hellais , I did 1 and 2. Removed the unused components and files (https://github.com/ooni/explorer/pull/785/commits/969574bc9753603e03a63d2d00591392ff117df8). I wanted to know what to do about some of the section headers, navigation and styling things.

  1. The navigation present here, since we removed the Networks section at last, the scroll link doesn't work anymore. image

  2. The section headers, are those all good or can we improve that part? Because it still doesn't match - In the Websites section, we got 2 headers of Website. Then the last 2 charts (Instant Messaging & Circumvention) are present inside Apps section.

GermaVinsmoke commented 2 years ago

About 3. Was thinking do we need to add the filter parameters in URL also? /country/CM?since=2022-08-01&until=2022-09-07 Just like we got on network page - /network/AS7018?since=2022-08-01&until=2022-09-07

hellais commented 1 year ago

The navigation present here, since we removed the Networks section at last, the scroll link doesn't work anymore.

Yeah I would say we just drop the networks tab until we re-add support for it.

The section headers, are those all good or can we improve that part? Because it still doesn't match - In the Websites section, we got 2 headers of Website.

Good catch. I think we would ideally at the very least drop the Websites heading from the Websites section, so we aren't repeating it twice.

Was thinking do we need to add the filter parameters in URL also?

Yeah I think that's a solid plan