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

feat: update header with new UI #312

Merged
merged 7 commits into from
May 1, 2024
Merged

feat: update header with new UI #312

merged 7 commits into from
May 1, 2024

Conversation

ErikSin
Copy link
Contributor

@ErikSin ErikSin commented May 1, 2024

Updated the header in creating/editting an observation to include new save button. Also replaces the native toast with a modal for confirming whether user wants to discard changes when trying to leave the edit screen.

image image

@ErikSin ErikSin changed the title chore: update header with new UI feat: update header with new UI May 1, 2024
Comment on lines -59 to +61
const handleDetailsPress = React.useCallback(() => {
navigation.navigate('ObservationDetails', {question: 1});
}, [navigation]);
// const handleDetailsPress = React.useCallback(() => {
// navigation.navigate('ObservationDetails', {question: 1});
// }, [navigation]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will be added really soon, so just commented out for linting purposes

Comment on lines 150 to 153
editDeviceInfoMutation.mutate,
editDeviceInfoMutation.isPending,
editDeviceInfoMutation,
nameHasChanges,
errorModal.openSheet,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was not playing well with the linter, so i just added the entire object

Copy link
Member

@achou11 achou11 May 1, 2024

Choose a reason for hiding this comment

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

if you extract the field first and use that, it should work e.g.

const {openSheet} = useBottomSheetModal({openOnMount: false});

i think adding the whole object is not desirable (in theory) because the effect will run if when one of its fields changes (e.g. isOpen). maybe it's not a big issue in practice though 🤷

@ErikSin ErikSin requested a review from achou11 May 1, 2024 20:35
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

Couple of non-blocking comments but otherwise lgtm

},
{
onPress: closeSheet,
text: formatMessage(m.discardCancel),
Copy link
Member

Choose a reason for hiding this comment

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

nit: Seeing the cancel action copy of Continue editing seems a little off when you're on the category chooser screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im going to leave this because we are planning on updating all the copy at a later time, so that can be dealt with then

Comment on lines 150 to 153
editDeviceInfoMutation.mutate,
editDeviceInfoMutation.isPending,
editDeviceInfoMutation,
nameHasChanges,
errorModal.openSheet,
Copy link
Member

@achou11 achou11 May 1, 2024

Choose a reason for hiding this comment

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

if you extract the field first and use that, it should work e.g.

const {openSheet} = useBottomSheetModal({openOnMount: false});

i think adding the whole object is not desirable (in theory) because the effect will run if when one of its fields changes (e.g. isOpen). maybe it's not a big issue in practice though 🤷

@ErikSin ErikSin merged commit 1dd8130 into main May 1, 2024
3 checks passed
@ErikSin ErikSin deleted the es/feat-discard-modal branch May 1, 2024 21:26
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