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

refactor: client app to use latest specifications #317

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

DenizYil
Copy link
Contributor

@DenizYil DenizYil commented Sep 3, 2023

  • Updates all packages used to their latest version. This included bumping and migrating Material UI from version 4 to 5. This change in itself is quite large and is what the majority is for. It required all files to be updated.
    • This creates a bit of a visual difference, but it follows their guidelines.
  • Also updated the theme specifications to use the latest DHI ones. This includes some small changes, e.g. font sizes, margins, etc. This also included some visual changes like changing the text field variants, etc.
  • Added Eslint so the code is consistent and follows the latest DHI practices.
  • Fixed a bug where the keys were often not used in the correct order
  • Tons of small optimizations here and there, plenty of work still left in this regard though.
  • Removed unused images
  • Removed custom webpack settings/compiler and used the default from CRA. As a future change, we could look into using Vite or something else.
  • Closes terracotta connect requesting metadata keys in wrong order #314 and Update terracotta interactive web client dependencies  #312
  • .. and more

@dionhaefner I had some issues running the connect command. If you're able to try that command (when giving the client a look), that would be really appreciated - but it should all still work.

@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

Merging #317 (97dcedf) into main (e6f1952) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

❗ Current head 97dcedf differs from pull request most recent head 159f2e9. Consider uploading reports for the commit 159f2e9 to get more accurate results

@@           Coverage Diff           @@
##             main     #317   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files          52       52           
  Lines        2335     2335           
  Branches      327      286   -41     
=======================================
  Hits         2291     2291           
  Misses         29       29           
  Partials       15       15           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dionhaefner
Copy link
Collaborator

Thanks @DenizYil 🎉

I'm seeing some typescript errors when trying to build the app on CI. Could you have a look at that?

@DenizYil
Copy link
Contributor Author

@dionhaefner sincere apologies for the delay with this. I had somehow completely missed all of this.

All the ESLint fixes should be fixed now and the build seems to complete successfully. Please let me know if there's anything else needed for this. I'll monitor this PR actively.

I'm not exactly sure why the Python pipelines are failing, but I guess that is more your expertise than mine (please let me know if there's any way that I can help).

@dionhaefner
Copy link
Collaborator

Unfortunately there seem to be more problems with the client. The basemap is broken, and the proportions look off (font size too large and margins too small).

Before

Screenshot 2023-09-20 at 14 02 20

After

Screenshot 2023-09-20 at 14 02 34

Is there any way you could implement only the bug fix to #314 in a separate PR that we can merge right away, then keep iterating on this?

@DenizYil DenizYil mentioned this pull request Sep 21, 2023
@DenizYil
Copy link
Contributor Author

Is there any way you could implement only the bug fix to #314 in a separate PR that we can merge right away, then keep iterating on this?

Done in #318. Good call.


For the feedback: I'll fix the basemap, not sure how that disappeared as it works locally, but will investigate tomorrow. For the proportions, it uses the latest DHI (and MUI) specifications, so some numbers were increased (e.g. base fonts). I wasn't sure whether or not to try to match the old one 1:1, but if that's what we decide, I can hardcode the numbers and change the styling, which should be straightforward and a relatively quick thing to change throughout.

@dionhaefner
Copy link
Collaborator

I'm not married to the original font size, but if we end up changing it we should also adjust some margins here and there to give the design some breathing room. Right now the UI looks crammed to me, especially around the contrast slider and in / around the "raster URL" / "Metadata" boxes.

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.

terracotta connect requesting metadata keys in wrong order
2 participants