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

Mortein 63 detail page navigation #19

Merged
merged 23 commits into from
Oct 2, 2024
Merged

Conversation

Amelia-C5
Copy link
Contributor

Added links to detail page.
Also changed the way routing was implemented in order to generate links based on the contents of db.json (i.e. for each plant device in db.json, new icon and link created).
Also attempted to fix some formatting issues with the homepage.

db.json Outdated Show resolved Hide resolved
@willsawyerrrr
Copy link
Contributor

Also changed the way routing was implemented in order to generate links based on the contents of db.json

What was wrong with the old method of dynamically routing these pages?

@Amelia-C5
Copy link
Contributor Author

The Routing for the detail page wasn't being generated for each device object in the json file. Instead it was the same detail Page for every device (Orange Tree data). I changed it so that each plant device object you click, you get routed to its relevant detail Page. In order to do this, I needed to initialise a list of routes using createBrowserRouter and also separately mapping each router object to another list. I then concatenated these together. This was the only way which seemed to work for generating new routes to detail pages using mapping.

@willsawyerrrr
Copy link
Contributor

It looks like you weren't passing the plant's ID into the PlantItem component here.

If you pass it into there, does your old routing work?

@Amelia-C5
Copy link
Contributor Author

No it still doesn't. I think it may be the way I have implemented the detail page. I do not fully understand how dynamic routing works so I have not implemented the detail page in the best way possible for dynamic routing. Due to using the db.json file for getting mock api data, I just fetched all this data at the App.jsx level and then passed it through the components. Looking online, it seems like some people fetch the data at the component level using the id from the routing. I can look into this further. Just for my own interest, is there any reason my way is worse than dynamically routing (E.g. fetching at the App level rather than for each component)? Is it easier to implement, more efficient, less updates to component renders needed? I can try to get the detail page working tomorrow with dynamic routing.

@Amelia-C5
Copy link
Contributor Author

I have figured out the issue - which was with the way I implemented the detail page. It's a quick fix though so I will implement my other changes in another branch and do a pull request for that one. Thanks for your help! I really appreciate it.

@Amelia-C5
Copy link
Contributor Author

I ended up re-implementing dynamic routing in this branch.

@willsawyerrrr
Copy link
Contributor

I just fetched all this data at the App.jsx level and then passed it through the components. Looking online, it seems like some people fetch the data at the component level using the id

If all of your data is at the App level, all state changes require the entire app to be rerendered. You may also store more data than needed if the top-level stores all data required to render all nested pages.

Consider having 10 plants to display at the top-level - this requires 10 names and 10 IDs. Now consider that you're also retrieving and storing the latest healthcheck datum for each plant. You're making 10 additional network requests and storing 10 healthcheck datum objects. If you only check on one of your plants, 9 of those requests and stored objects are completely useless.

@Amelia-C5
Copy link
Contributor Author

I just fetched all this data at the App.jsx level and then passed it through the components. Looking online, it seems like some people fetch the data at the component level using the id

If all of your data is at the App level, all state changes require the entire app to be rerendered. You may also store more data than needed if the top-level stores all data required to render all nested pages.

Consider having 10 plants to display at the top-level - this requires 10 names and 10 IDs. Now consider that you're also retrieving and storing the latest healthcheck datum for each plant. You're making 10 additional network requests and storing 10 healthcheck datum objects. If you only check on one of your plants, 9 of those requests and stored objects are completely useless.

That makes sense. It was re-rendering a lot. Thanks for your help. I have reverted it back to dynamic for this branch. I am not fetching at the detail page level yet but will when I get the api working with the app.

@Amelia-C5 Amelia-C5 merged commit 74b30d3 into main Oct 2, 2024
3 checks passed
@Amelia-C5 Amelia-C5 deleted the MORTEIN-63-Detail-Page-Navigation branch October 2, 2024 09:41
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.

3 participants