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 "Verify Inference" Checkmark #1047

Merged
merged 4 commits into from
Oct 5, 2023

Conversation

the-bay-kay
Copy link
Contributor

Work discussed in Issue 979 !

  • The "verify trip" button now only only appears if both Mode and Purpose have verifiable inferences
  • The button has been changed for clarity (centered, larger icon, color matches the verification color)

See [Issue e-mission#979](e-mission/e-mission-docs#979 (comment)) for more details.
Only slight difference in comment; removed the `-0.2rem` in this commit,
the buttons look crowded otherwise.
Updated the button to only appear if you can verify the trip.
Button color now matches the "inferred trip" data.
@the-bay-kay the-bay-kay marked this pull request as ready for review October 2, 2023 19:10
Copy link
Collaborator

@JGreenlee JGreenlee left a comment

Choose a reason for hiding this comment

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

Great work! I have a couple minor suggestions and then this looks good to me.

www/js/survey/multilabel/MultiLabelButtonGroup.tsx Outdated Show resolved Hide resolved
www/js/survey/multilabel/MultiLabelButtonGroup.tsx Outdated Show resolved Hide resolved
the-bay-kay and others added 2 commits October 3, 2023 08:17
See conversation in PR
 `rem` doesn't work properly in react-native, updated

Co-authored-by: Jack Greenlee <JackAGreenlee@gmail.com>
iconButtonProp & Border Color
Changed prop to use native property, adjusted border color.

Co-authored-by: Jack Greenlee <JackAGreenlee@gmail.com>
@JGreenlee
Copy link
Collaborator

@the-bay-kay Can you move this into "Ready for review" on https://github.com/orgs/e-mission/projects/1/views/1 ?
If I hadn't been keeping an eye on the related issue, I might have missed this. And it will not get merged unless it's in "Ready for review".

disabled={trip.verifiability != 'can-verify'}
style={{width: 20, height: 20, margin: 3}}/>
</View>
{trip.verifiability === 'can-verify' && (
Copy link
Contributor

@shankari shankari Oct 5, 2023

Choose a reason for hiding this comment

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

@the-bay-kay I see that this logic is unmodified, so I am going to merge it. But I would like to understand what it does, maybe illustrated with a short video.

From the code, it looks like if either button is yellow, we can "confirm" it. So if I had one yellow label and one blank label, I could press the confirm button and it would turn the yellow -> blue but leave the blank label unchanged.

Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My apologies for the late reply! Here are two videos, comparing the old UI and the changes this added.

beforeVerify.mov
VerificationCheckmark.mp4

@shankari shankari merged commit cc0e419 into e-mission:master Oct 5, 2023
4 checks passed
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.

3 participants