NMF-earth / nmf-app

Understand and reduce your carbon footprint 🌱 iOS & Android.
https://nmf.earth
GNU General Public License v3.0
498 stars 156 forks source link

144 emission info #212

Closed adamfitzgibbon closed 3 years ago

adamfitzgibbon commented 3 years ago

Resolves #144

Pulled the Emission Info button into the shared components so that it could be reused for existing emissions and added it to EmissionItem.

Also added a test for coverage :)

PierreBresson commented 3 years ago

@adamfitzgibbon thanks for you pr and sorry for the merge conflict (but should be easy to fix). Moving to components is the right way to go, you are correct.

However, this pr doesn't answer don't answer this issue. Methodology issue/pr is similar technically but that's all. This concerns existing emissions. Depending on the emission type, clicking on the button should open a modal with markdown in it, describing the emission (this text will be done later on).

adamfitzgibbon commented 3 years ago

@PierreBresson oh I didn’t realize. Do you already have those markdown files or at least the text you want for each emission type?

PierreBresson commented 3 years ago

@adamfitzgibbon I don't have yet all the files, but I've create a chocolate.md as an example for you so you can have better idea of the implementation. Check https://github.com/NMF-earth/nmf-app/commit/f7a461688ab510561f4b72c1619bc8763936319c

adamfitzgibbon commented 3 years ago

Ok sounds good. I'll update the button to take a prop to switch between the desired md file and add some more placeholders and we can add those in later.

adamfitzgibbon commented 3 years ago

@PierreBresson do we have an established pattern for setting application state? We need the button to be aware of the currently selected emission type and I'm not sure there's a good way to pass props to the header since it isn't part of the component

PierreBresson commented 3 years ago

You should be able to extract the id of the emission thanks to the nav props and then use the ducks to get the emission type, just like in EmissionItemScreen :

  const route = useRoute();
  const emissionId = pathOr("", ["params", "id"], route); // get id

  const emission = useSelector((state) =>
    emissions.selectors.getEmissionById(state, emissionId)
  ); // get emission

Maybe there is another way to do it

adamfitzgibbon commented 3 years ago

Thanks, that works really well! I have to call it quits for the night, but what I have pushed up has most of the functionality in place. If there is a current emission type it is successfully updating the title of the modal to the emission type, otherwise (like in the AddEmission flow) it defaults to the existing "Methodology".

For some reason the params were not making it into the HtmlView component (I renamed the Methodology component but am open to suggestions here) so I still need to figure that piece out but after that it's basically good to go.

I'll finish that up and add some tests either tomorrow or the next day depending on when I have time. Sorry this has gotten kind of big! For the time being, if you have any feedback here it would be very welcome since there's a lot going on.

adamfitzgibbon commented 3 years ago

@PierreBresson Thanks so much for all the feedback! Got all of it updated. Also figured out my earlier issue: I had to tell the navigator to specifically pass the params into the child screen. Sometimes fully reading the docs instead of skimming for specific info pays off 😅

Let me know if I misunderstood anything or you see anything else. Otherwise this is looking pretty good. I'll add a couple tests tomorrow.

adamfitzgibbon commented 3 years ago

Here’s a screenshot of what the modal looks like from the emission item page. I left some placeholder text in the body and we can make an issue to wire those up to the corresponding Json file once we have those. Or if you’d like I can change it to first look for a file before inserting the placeholder so we can have individual issues for writing the docs and they’ll automatically fill in as they’re added.

38CD3E1E-8763-4B92-ACBC-FBBA9B522760

adamfitzgibbon commented 3 years ago

Added an additional test and renamed a test I missed. Should be good to go 👍

PierreBresson commented 3 years ago

@adamfitzgibbon Good job, thanks 🤗