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

Feature/react ts client preview #194

Merged
merged 35 commits into from
Apr 23, 2021
Merged

Conversation

bertearazvan
Copy link
Contributor

@bertearazvan bertearazvan commented Apr 14, 2021

This PR aims to rewrite the Terracotta preview app using React and Typescript based on the create-react-app boilerplate.

  • it adds the webpack configuration for local development and production build.
  • it adds the flask script to run the app
  • it recreates the app using the same kind of UI as satlas.dk

Close #195
Close #140

@dionhaefner dionhaefner marked this pull request as draft April 14, 2021 14:31
@bertearazvan bertearazvan requested a review from rihr April 22, 2021 14:03
@bertearazvan
Copy link
Contributor Author

d6aeeff33e43970ba3e26227f36d7fa2

@dionhaefner
Copy link
Collaborator

Are you planning to address some of our long-standing issues? #91 #135 #140 #182

@bertearazvan
Copy link
Contributor Author

Probably #140 is fixed(currently you can only slide, and not use an input for those min/max values - unsure if this is an issue. The precision of the slider for singleband values is 0.01, not sure if it should be more precise than this) and I'll look into #182, the other two seem a bit complex for this PR.

There are discussions going around #91 about having a client library that wraps TC with a Map provider - probably through deck.gl. Stay tuned

As about #135, hmm interesting feature, sparks my interest so I might look into it, but as a separate PR.

@dionhaefner
Copy link
Collaborator

OK. I personally prefer entering values to sliding, but your call if you want to support that.

#140 was not about this, though. The bug arose when a raster had a constant value, e.g. all 0. Then, the slider would break. So you should probably test this case to make sure it's fixed.

@codecov
Copy link

codecov bot commented Apr 23, 2021

Codecov Report

Merging #194 (ad140a7) into main (7653467) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #194   +/-   ##
=======================================
  Coverage   98.36%   98.36%           
=======================================
  Files          45       45           
  Lines        2200     2200           
  Branches      273      273           
=======================================
  Hits         2164     2164           
  Misses         20       20           
  Partials       16       16           
Impacted Files Coverage Δ
terracotta/client/flask_api.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7653467...ad140a7. Read the comment docs.

terracotta/client/app/.eslintcache Outdated Show resolved Hide resolved
terracotta/client/app/src/App.tsx Show resolved Hide resolved
terracotta/client/app/src/colormap/DatasetsColormap.tsx Outdated Show resolved Hide resolved
terracotta/client/app/src/colormap/SinglebandSelector.tsx Outdated Show resolved Hide resolved
terracotta/client/app/src/map/Map.tsx Outdated Show resolved Hide resolved
terracotta/client/app/src/colormap/SinglebandSelector.tsx Outdated Show resolved Hide resolved
@bertearazvan bertearazvan marked this pull request as ready for review April 23, 2021 14:16
@bertearazvan bertearazvan merged commit 9dc3972 into main Apr 23, 2021
@bertearazvan bertearazvan deleted the feature/react-ts-client-preview branch April 23, 2021 14:21
@dionhaefner
Copy link
Collaborator

Thanks for the implementation, it sure looks more professional now.

I just tried it, and I do have some comments:

  • The info text when you click on details seems superfluous now. Why not just remove it? It's hard to find anyway.
  • The bold /apidoc in the details section looks weird to me.
  • Raster listing starts at page 0, and no indication how many pages there are.
  • No padding between metadata and preview image looks cramped.
  • What happened to the zoom to the dataset? Imaging you have a raster of one yard in New Zealand, how are you ever going to find it on the world map? Just saw that it does zoom in, but not if you select the first dataset in the list 🤔
  • The function of the "change base map" button is not obvious before you click on it. Couldn't this be "satellite view" / "street view" with a matching icon instead?
  • Sidebar looks very large on smaller / non-maximized windows, and is not resizable anymore:

Screen Shot 2021-04-26 at 10 11 13

  • I just saw that you can minimize the sidebar. That arrow is hard to find. Couldn't this be on the left side of the sidebar?
  • RGB view is broken, there is no way to select different rasters for the bands.
  • I don't think the RGB contrast sliders work as they should. They just have a range of 0-255 instead of the range of the rasters.
  • Key descriptions are not part of the UI anymore?
  • Some more vertical breathing room would be nice.

No one has asked for my review, so you can do with this feedback whatever you want.

@j08lue
Copy link
Collaborator

j08lue commented Apr 26, 2021

Thanks for the ex post facto review, @dionhaefner. 😄 Thought it was nice to push the remake out there, great you are already picking up the revision on another PR, @bertearazvan.

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.

Rewrite TC preview app Preview app does not support datasets where min == max
4 participants