-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: show large suggestion menu only if there is space #28927
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4d5e108
fix: show large suggestion menu only if there is space
allroundexperts 54fa3c9
feat: merge with main
allroundexperts 342c592
feat: encapsulate chatItemComposeSecondaryRow styles
allroundexperts c1e5a56
feat: move the hasEnoughSpaceForLargeSuggestion to a util fn
allroundexperts 477a03e
feat: make the condition in spaceAvailableForLargeSuggestionMenu more…
allroundexperts d14442a
rename the spaceAvailableForLargeSuggestionMenu fn
allroundexperts af47c4c
feat: use total suggestions to calculate the menu height
allroundexperts 32d8147
feat: merge with main
allroundexperts 410ae8c
feat: merge with main
allroundexperts File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,6 @@ import React, {useRef, useCallback, useImperativeHandle} from 'react'; | |
import PropTypes from 'prop-types'; | ||
import SuggestionMention from './SuggestionMention'; | ||
import SuggestionEmoji from './SuggestionEmoji'; | ||
import useWindowDimensions from '../../../../hooks/useWindowDimensions'; | ||
import * as SuggestionProps from './suggestionProps'; | ||
|
||
const propTypes = { | ||
|
@@ -12,11 +11,15 @@ const propTypes = { | |
/** Function to clear the input */ | ||
resetKeyboardInput: PropTypes.func.isRequired, | ||
|
||
/** Is auto suggestion picker large */ | ||
isAutoSuggestionPickerLarge: PropTypes.bool, | ||
|
||
...SuggestionProps.baseProps, | ||
}; | ||
|
||
const defaultProps = { | ||
forwardedRef: null, | ||
isAutoSuggestionPickerLarge: true, | ||
}; | ||
|
||
/** | ||
|
@@ -25,10 +28,24 @@ const defaultProps = { | |
* | ||
* @returns {React.Component} | ||
*/ | ||
function Suggestions({isComposerFullSize, value, setValue, selection, setSelection, updateComment, composerHeight, forwardedRef, resetKeyboardInput, measureParentContainer}) { | ||
function Suggestions({ | ||
isComposerFullSize, | ||
value, | ||
setValue, | ||
selection, | ||
setSelection, | ||
updateComment, | ||
composerHeight, | ||
forwardedRef, | ||
resetKeyboardInput, | ||
measureParentContainer, | ||
isAutoSuggestionPickerLarge, | ||
}) { | ||
const suggestionEmojiRef = useRef(null); | ||
const suggestionMentionRef = useRef(null); | ||
|
||
const getSuggestions = useCallback(() => suggestionEmojiRef.current.getSuggestions() || suggestionMentionRef.current.getSuggestions(), []); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This caused a regression as null handling is missing: |
||
|
||
/** | ||
* Clean data related to EmojiSuggestions | ||
*/ | ||
|
@@ -71,16 +88,11 @@ function Suggestions({isComposerFullSize, value, setValue, selection, setSelecti | |
triggerHotkeyActions, | ||
updateShouldShowSuggestionMenuToFalse, | ||
setShouldBlockSuggestionCalc, | ||
getSuggestions, | ||
}), | ||
[onSelectionChange, resetSuggestions, setShouldBlockSuggestionCalc, triggerHotkeyActions, updateShouldShowSuggestionMenuToFalse], | ||
[onSelectionChange, resetSuggestions, setShouldBlockSuggestionCalc, triggerHotkeyActions, updateShouldShowSuggestionMenuToFalse, getSuggestions], | ||
); | ||
|
||
const {windowHeight, isSmallScreenWidth} = useWindowDimensions(); | ||
|
||
// the larger composerHeight the less space for EmojiPicker, Pixel 2 has pretty small screen and this value equal 5.3 | ||
const hasEnoughSpaceForLargeSuggestion = windowHeight / composerHeight >= 6.8; | ||
const isAutoSuggestionPickerLarge = !isSmallScreenWidth || (isSmallScreenWidth && hasEnoughSpaceForLargeSuggestion); | ||
|
||
const baseProps = { | ||
value, | ||
setValue, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the most interesting part that wasn't present in the proposal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There are some places where we also show a
Pay With Expensify
button in the header. This causes the height of the header to be different from what it is normally. To cater for this edge case, we need to get header height accurately.