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

fix: Various improvements to the Observations screen #373

Merged
merged 4 commits into from
Jul 17, 2020

Conversation

okdistribute
Copy link
Contributor

Updates to the latest react-mapfilter

fix: Allow editing Select Multiple components (fixes #372)
fix: Select one editing should not crash Mapeo (fixes #371)
fix: Collapse sub-menus by defauilt in Observations (fixes #348)
feat: Support localized type fields
feat: Support singleline text fields

fix: Allow editing Select Multiple components (fixes #372)
fix: Select one editing should not crash Mapeo (fixes #371)
fix: Collapse sub-menus by defauilt in Observations (fixes #348)
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

@okdistribute I dropped the ball reviewing the multi-select implementation. There is a bug in both multi-select and single-select when selecting from a list of options that is defined with [{label: "Item label", value: "item_value"}]. However this bug existed before because of a silly mistake by me (took me ages to spot this today — I'm doing the comparison as the first function arg, vs. passing two args).

The desired behaviour is that the user is shown option.label but the value stored in the data is option.value. This has not been an issue in the past, because we do not have presets in the field which are using this way of defining options for select fields. However @jencastrodoesstuff is just updating the ECA presets to use this format. This way of defining options is valuable because it allows us to change what is shown to the user whilst not changing what is actually stored on the data, e.g. we might have an option for a field "type of contamination" and options.value might be spill, but options.label could be Oil Spill, then later the community might decide that this needs clarification and we change the option to Crude Oil Spill, but we want the value to remain spill for backwards compatibility with data already collected.

This is how select fields work on Mobile (although, like Desktop, this is critically missing unit tests). If Mapeo Desktop does not have this same behaviour, editing select and multi-select fields on Desktop is going to result in unexpected data in the database (the value will be the label, and the filters will not work correctly).

@okdistribute
Copy link
Contributor Author

okdistribute commented Jul 10, 2020

@gmaclennan ok great I will update to presets that include this label/value and patch a fix in react-mapfilter

@okdistribute
Copy link
Contributor Author

@gmaclennan fix ready! digidem/react-mapfilter#102

@gmaclennan
Copy link
Member

Thanks @okdistribute I left a review for some small fixes on the PR. Also spotted another small bug and fixed it: digidem/react-mapfilter#103

@okdistribute
Copy link
Contributor Author

Getting a flow bug here trying to integrate react-mapfilter@3.2.3 --

Uncaught /home/okdistribute/dev/mapeo-desktop/node_modules/react-mapfilter/commonjs/ObservationDialog/Select.js:128
): %checks
 ^

SyntaxError: Unexpected token :

Could it be an already known bug? facebook/flow#8411

@gmaclennan
Copy link
Member

Yuk what a pain. I checked and this is not being stripped as it should be at build time, and I also tried updating all the babel modules to latest. As a fix I just removed the flow checks for this piece of code and published as v3.2.4. This is one area where TypeScript is much better — type predicates are more powerful I think than what Flow offers.

@okdistribute okdistribute merged commit a77019b into master Jul 17, 2020
@okdistribute okdistribute deleted the mapfilter-fixes branch July 17, 2020 03:45
@okdistribute
Copy link
Contributor Author

Thanks @gmaclennan !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants