search

DigitalCommons / land-explorer-front-end

React app for the Land Explorer front end
http://landexplorer.cc
GNU Affero General Public License v3.0
1 stars 0 forks source link

289 backsearch p1 fe error handling #299

Closed ms0ur1s closed 6 months ago

ms0ur1s commented 7 months ago

What? Why?

Closes #289

Unhandled errors cause LX to white screen with no error message and no recourse but to click the back button, which takes you away from the app.

What should we test?

Repeat tests that previously triggered the white screen error. (@lin-d-hop, not sure how to do this without uncommenting the select all button)

ms0ur1s commented 6 months ago

Hey @ms0ur1s , this is looking great, and thanks for taking the ticket from me!

I have a couple of (very minor) comments, since Lynne asked us to be strict with our reviews lol, but pls know I'm being pedantic:

  1. I think it would make more sense for the new ErrorFallback to be in the pages/ folder, with FourOhFour, rather than it's own errorFallback folder. We generally try to organise components/ into folders based on their position on the screen, so it's easier to find stuff. Also, maybe the name could be a bit more descriptive (e.g. SomethingWentWrong) so it's easier to understand what it is without reading the file?
  2. In the Git diff, I'm seeing lots of indentation changes in unrelated code, which makes the reviewing and tracking past changes more difficult. I think because your IDE is forcing a tab size of 2. In VS Code, I think you can set "editor.detectIndentation" to "true" in your settings to stop this happening in the future (don't worry about changing back the indentation in this PR)

Other than that, I think it's good to merge :)

Nice one @rogup, all makes sense man. I'll make those changes and then re-commit.

ms0ur1s commented 6 months ago

@rogup, made the changes. It looked as if "editor.detectIndentation": true was already on, though. Could my default formatter also be causing a problem?

image

rogup commented 6 months ago

@ms0ur1s I'm not sure, but yeah it looks like that might be the case. There's a ticket here to set a formatting standard for the whole codebase (probably with Prettier), so let's just wait until that gets completed. Then hopefully the formatting won't keep changing when different developers work on it :) https://github.com/DigitalCommons/land-explorer-front-end/issues/209

lin-d-hop commented 6 months ago

Loving the reviewing here :green_heart:

@rogup Shall we uncomment the Select All button alongside this? Would be great to text and if this PR passes I'd be happy to release the select all button alongside :)

lin-d-hop commented 6 months ago

Definitely an improvement. The system gets very laggy to the point of unresponsive. However I think we can live with it being slow on big datasets and wait for people to complain about it.