-
Notifications
You must be signed in to change notification settings - Fork 74
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ 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 |
Thanks @DenizYil 🎉 I'm seeing some typescript errors when trying to build the app on CI. Could you have a look at that? |
@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). |
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). BeforeAfterIs 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. |
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. |
@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.