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

Updated packages #73

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Updated packages #73

merged 1 commit into from
Nov 16, 2023

Conversation

PasiVuohijoki
Copy link
Contributor

Excplicitly added crossOrigin={undefined} into dateinput form field to make typescript compiler happy
Added resolver to prevent @react/types version 18 to be used. It breaks quite a lot of components currently

@EmiliaMakelaVincit
Copy link
Contributor

EmiliaMakelaVincit commented Sep 28, 2023

I could test this but I think the point has been lost a bit here. I see no changes to our main dependency versions in package.json itself, so I assume this patch is only applying the latest hotfix/minor versions and not actually bringing our dependencies fully up to date.

@PasiVuohijoki PasiVuohijoki force-pushed the fix/update-packages branch 2 times, most recently from 8e366fb to 7dae7cf Compare October 9, 2023 15:30
@PasiVuohijoki PasiVuohijoki force-pushed the fix/update-packages branch 3 times, most recently from 1ccde9e to 6f1c7a9 Compare October 17, 2023 10:24
Copy link
Contributor

@EmiliaMakelaVincit EmiliaMakelaVincit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review is only for the actual code part. While I did verify I could get the UI up after installing Node 18, my hands-on testing was somewhat limited today, but I'll go through all the key flows in detail on Monday.

This generally looks good. I think most of my comments were on shortcuts on issues I know to be difficult to fix, and it's likely fine to keep them as they are, just with the justification made clear in the context of that fix.

src/_app.scss Outdated Show resolved Hide resolved
src/a11y/ScreenReaderText.tsx Outdated Show resolved Hide resolved
src/map/MapReadyHandler.tsx Show resolved Hide resolved
src/footer/footer.tsx Outdated Show resolved Hide resolved
src/i18n/en/common.json Show resolved Hide resolved
src/main.scss Outdated Show resolved Hide resolved
src/application/components/applicationFormSubsection.tsx Outdated Show resolved Hide resolved
src/topNavigation/topNavigation.tsx Show resolved Hide resolved
src/topNavigation/topNavigation.tsx Show resolved Hide resolved
@EmiliaMakelaVincit
Copy link
Contributor

EmiliaMakelaVincit commented Oct 30, 2023

I added two clarification comments to the previous set in the morning, and from the hands-on test I have two more things to mention:

  • This warning gets sent into the console log: createSlice.ts:335 The object notation for createSlice.extraReducers is deprecated, and will be removed in RTK 2.0. Please use the 'builder callback' notation instead: https://redux-toolkit.js.org/api/createSlice. Do you think this is something that could also be changed in this update (or a follow-up)? It is related to updated packages, but I don't know how much work it'd be to fix now.
  • The maximum height of the map component in the two map views needs to be adjusted, as with the new taller header the layers button gets "hidden" a scroll away once again even though it was fixed before.

@EmiliaMakelaVincit
Copy link
Contributor

Oh, third point I just discovered: The popup that appears after you select a target still assumes that the header is sticky even though it isn't. If the page has been scrolled, it floats at an awkward distance from the top of the page.

@PasiVuohijoki PasiVuohijoki force-pushed the fix/update-packages branch 2 times, most recently from ffd5c7a to 94dd762 Compare November 14, 2023 14:37
Copy link
Contributor

@EmiliaMakelaVincit EmiliaMakelaVincit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again, here's the code-side review for now, and the hands-on review will come a bit later. Seemed pretty good, half of my comments were questions that are likely fine as is already.

README.md Outdated Show resolved Hide resolved
Dockerfile.dev Outdated Show resolved Hide resolved
src/App.tsx Show resolved Hide resolved
src/api/callApi.ts Show resolved Hide resolved
src/application/components/applicationFormSubsection.tsx Outdated Show resolved Hide resolved
src/form/FileUploadsContext.tsx Outdated Show resolved Hide resolved
src/frontPage/_frontPage.scss Outdated Show resolved Hide resolved
src/mapSearch/mapSearchComponent.tsx Outdated Show resolved Hide resolved
src/topNavigation/topNavigation.tsx Outdated Show resolved Hide resolved
src/boxGrid/_boxGridBox.scss Show resolved Hide resolved
@EmiliaMakelaVincit
Copy link
Contributor

From the last hands-on test, these haven't been changed:

  • The map view height hasn't been adjusted; the layer selection button and the bottom of the view are still a scroll away when at the top of the page
  • No change or comment was made towards the awkward positioning of the toast popup

One new observation from this round:

  • Leaflet related button icons aren't showing up for me (Leaflet's map layer button, as well as all leaflet-draw button icons). It seems that the locations are compiled to be relative and resolve to start from the initial path on page load, as the file for the former is being looked up from "http://localhost:4000/aluehaku/hakemus/images/layers-2x.png" for instance if I reload on the first stage of area search flow.

@PasiVuohijoki PasiVuohijoki force-pushed the fix/update-packages branch 2 times, most recently from 06581d9 to c67bc46 Compare November 15, 2023 12:48
Transferred from create_react_app to vite
Copy link
Contributor

@EmiliaMakelaVincit EmiliaMakelaVincit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go 🎉

@PasiVuohijoki PasiVuohijoki merged commit 5468446 into develop Nov 16, 2023
2 checks passed
@PasiVuohijoki PasiVuohijoki deleted the fix/update-packages branch November 16, 2023 09:01
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.

2 participants