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

💅 Label Screen cleanup & polishing #998

Conversation

JGreenlee
Copy link
Collaborator

After converting the Label Screen UI to React components with #993, there are several items to address.

This PR will fix regressions, tidy, and continue to improve usability.

To avoid cluttering app.js more than necessary, I am extracting the logic to initialize i18next to a new file called i18nextInit.ts, which just performs the same initialization logic and exports the initialized instance of i18next.

In tsconfig.json, `resolveJsonModule` was set `true` so that the JSON translation files can be imported in TypeScript.
and alias windowDimensions 'width' as 'windowWidth' for readability/clarity
@JGreenlee
Copy link
Collaborator Author

JGreenlee commented Jun 29, 2023

The above commits are minor cleanups/improvements I already had, but didn't want to tack onto #993 while it was being reviewed.

The most notable thing is that I implemented caching for the address names we fetch from OSM. I've always found it annoying that they're retrieved again on every app load.

@shankari
Copy link
Contributor

The most notable thing is that I implemented caching for the address names we fetch from OSM. I've always found it annoying that they're retrieved again on every app load.

Note that we are also signing up for a paid subscription to geofabrik.de. Once that is done, we can fill in the addresses on the server. Regardless, we will still need direct client calls for draft trips, so this is a useful fix. Just might be less cool down the road.

@JGreenlee
Copy link
Collaborator Author

Sounds cool!

I think this was still largely worthwhile because now we have a lightweight storage method to use instead of localstorage (on a true native app, localstorage will not exist) and I have figured out a process for caching, which we could do for other network calls down the line

The `key` prop needs to go on the <View> here because that is the element being repeated; not the IconButton.
The filter state that is applied initially should be the first one in the list. This updates FilterSelect to reflect that.
This was made translatable in 36a1aaf, but I overwrote it when I merged with 14e8a67 and I must have incorrectly resolved the merge conflicts for this line in particular.
@JGreenlee JGreenlee force-pushed the label-screen-migration-cleanup branch from 7a799fe to 7534f86 Compare June 29, 2023 21:23
Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

LGTM!

@shankari shankari marked this pull request as ready for review June 29, 2023 21:30
@shankari shankari merged commit d4079c3 into e-mission:framework_migration_prep Jun 29, 2023
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