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

Make the textinput visible when the keyboard is shown to take notes. #30

Merged
merged 2 commits into from
May 21, 2022

Conversation

pranavcd
Copy link
Collaborator

The behavior is different for ios and android for the keyboard avoiding view with the LessonPrimaryLayout. This change takes into consideration the available window height to adjust the position of the textinput closer to the keyboard.

Comment on lines 79 to 90
function onKeyboardDidShow(e: KeyboardEvent) {
const availableWindowHeight =
Dimensions.get('window').height - e.endCoordinates.height;
if (availableWindowHeight > DEFAULT_AVAILABLE_WINDOW_HEIGHT_THRESHOLD) {
setTextVoiceInputBottom(0);
}
}

function onKeyboardDidHide() {
const availableWindowHeight = Dimensions.get('window').height;
setTextVoiceInputBottom(DEFAULT_TEXT_VOICE_INPUT_BOTTOM);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two functions should be defined with useCallback so their identity doesn't change from render to render, and they should be listed as dependencies of the useEffect on line 105. Even though this should work fine without doing that, it's very easy to unintentionally introduce bugs the way this is currently written:

If these stay as fresh function declarations on every render but they're not listed in the dependency array of the useEffect on line 105, then the useEffect call might be using stale versions of the functions.

If you add the function names to the dependency array but don't use useCallback then you'll be causing the effect to run after every render, which is not ideal from a performance perspective.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
function onKeyboardDidShow(e: KeyboardEvent) {
const availableWindowHeight =
Dimensions.get('window').height - e.endCoordinates.height;
if (availableWindowHeight > DEFAULT_AVAILABLE_WINDOW_HEIGHT_THRESHOLD) {
setTextVoiceInputBottom(0);
}
}
function onKeyboardDidHide() {
const availableWindowHeight = Dimensions.get('window').height;
setTextVoiceInputBottom(DEFAULT_TEXT_VOICE_INPUT_BOTTOM);
}
const onKeyboardDidShow = useCallback((e: KeyboardEvent) => {
const availableWindowHeight =
Dimensions.get('window').height - e.endCoordinates.height;
if (availableWindowHeight > DEFAULT_AVAILABLE_WINDOW_HEIGHT_THRESHOLD) {
setTextVoiceInputBottom(0);
}
}, []);
const onKeyboardDidHide = useCallback(() => {
const availableWindowHeight = Dimensions.get('window').height;
setTextVoiceInputBottom(DEFAULT_TEXT_VOICE_INPUT_BOTTOM);
}, []);

showSubscription.remove();
hideSubscription.remove();
};
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}, []);
}, [onKeyboardDidHide, onKeyboardDidShow]);

@@ -48,11 +54,15 @@ export default function ImageCaptureLessonScreen({

const [imageCaptured, setImageCaptured] = useState<Boolean>(false);
const [showNavigation, setShowNavigation] = useState<boolean>(true);
const [showChatBubble, setShowChatBubble] = useState<boolean>(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this area can show multiple bubbles, so showChats or showChatArea would be a little clearer

@pranavcd pranavcd merged commit 3345fb4 into main May 21, 2022
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