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

[MRXN23-604] fixes target/spf values after editing one of them #1662

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

andresgnlez
Copy link
Member

Substitute this line for a meaningful title for your changes

Overview

This PR fixes an issue were the features listed were defaulted to the app's default values (for target and spf) when one of the features was edited by selecting it (through the checkbox) and clicking on the edit button in the floating menu.

The PR also displays the current values for target and spf when the user edits their values instead of defaulting to the app's default values. If more than one features are selected to edit, then the app's default values will display though.

image

Designs

Link to the related design prototypes (if applicable)

Testing instructions

Please explain how to test the PR: ID of a dataset, steps to reach the feature, etc.

Feature relevant tickets

https://vizzuality.atlassian.net/browse/MRXN23-604


Checklist before submitting

  • Meaningful commits and code rebased on develop.
  • If this PR adds feature that should be tested for regressions when
    deploying to staging/production, please add brief testing instructions
    to the deploy checklist (docs/deployment-checklist.md)
  • Update CHANGELOG file

@andresgnlez andresgnlez added the Frontend Everything related frontend label Mar 11, 2024
@andresgnlez andresgnlez self-assigned this Mar 11, 2024
Copy link

vercel bot commented Mar 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marxan ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 4:07pm

@hotzevzl
Copy link
Member

@andresgnlez thanks - I didn't really get the magic behind the change, but it does what it should do - values stay as they are unless they are touched/changed.

I really love the solution you devised to clarify bulk edits! This is vastly better than the original one based on focus events, and way clearer.

A couple of minor thing about the bulk edit popup:

  • the target % should only allow decimal numeric values between 0 and 100 inclusive
  • spf does not need to be capped at 1; in fact the Marxan manual is silent on min/max for this, so I think in practice users may want to enter any numeric value (decimal or integer) as they see fit - the value is used as a multiplier so I think it should not be zero, and I don't see when a negative spf may make sense, but I'd still just constrain this to any numeric value, decimal or integer

Other than this, all looks good to me, thanks, and well done for the big leap forward on the bulk edit UX! 🙌🏼

@andresgnlez
Copy link
Member Author

@hotzevzl noted! I added the validations to the fields. I'll be merging this!

@andresgnlez andresgnlez merged commit 09144a8 into develop Mar 12, 2024
13 checks passed
@andresgnlez andresgnlez deleted the MRXN23-604-fe-scenario-features-in branch March 12, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Everything related frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants