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.

Sign up for GitHub

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 9 commits into from
Oct 12, 2023
Merged

fix: show large suggestion menu only if there is space #28927

Show file tree
Hide file tree
Changes from 1 commit
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 Oct 5, 2023
54fa3c9
feat: merge with main
allroundexperts Oct 8, 2023
342c592
feat: encapsulate chatItemComposeSecondaryRow styles
allroundexperts Oct 8, 2023
c1e5a56
feat: move the hasEnoughSpaceForLargeSuggestion to a util fn
allroundexperts Oct 9, 2023
477a03e
feat: make the condition in spaceAvailableForLargeSuggestionMenu more…
allroundexperts Oct 9, 2023
d14442a
rename the spaceAvailableForLargeSuggestionMenu fn
allroundexperts Oct 9, 2023
af47c4c
feat: use total suggestions to calculate the menu height
allroundexperts Oct 9, 2023
32d8147
feat: merge with main
allroundexperts Oct 11, 2023
410ae8c
feat: merge with main
allroundexperts Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/libs/SuggestionUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,18 @@ function trimLeadingSpace(str) {
return str.slice(0, 1) === ' ' ? str.slice(1) : str;
}

export {getMaxArrowIndex, trimLeadingSpace};
/**
* Checks if space is available to render large suggestion menu
* @param {Number} listHeight
* @param {Number} composerHeight
* @returns {Boolean}
*/
function spaceAvailableForLargeSuggestionMenu(listHeight, composerHeight) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This util won't hurt! But I meant something else, I wasn't clear enough

Copy link
Contributor

@cubuspl42 cubuspl42 Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also, the name sounds like it returned some calculated space; maybe it could be named hasEnoughSpaceForLargeSuggestionMenu or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled!

return (
listHeight - composerHeight - (CONST.CHAT_FOOTER_SECONDARY_ROW_HEIGHT + 2 * CONST.CHAT_FOOTER_SECONDARY_ROW_PADDING) >
Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that this expression spans across three lines, it's really really wide.

So I was thinking if extracting both left and right sides of > to a const variable above wouldn't increase the readability

Copy link
Contributor

Choose a reason for hiding this comment

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

and maybe also some other components, like const totalSuggestionsHeight = CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_VISIBLE_SUGGESTIONS_IN_CONTAINER * CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTION_ROW_HEIGHT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled!

CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_VISIBLE_SUGGESTIONS_IN_CONTAINER * CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTION_ROW_HEIGHT +
CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTER_INNER_PADDING * 2
);
}

export {getMaxArrowIndex, trimLeadingSpace, spaceAvailableForLargeSuggestionMenu};
7 changes: 3 additions & 4 deletions src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import usePrevious from '../../../../hooks/usePrevious';
import * as EmojiUtils from '../../../../libs/EmojiUtils';
import * as User from '../../../../libs/actions/User';
import * as ReportUtils from '../../../../libs/ReportUtils';
import * as SuggestionUtils from '../../../../libs/SuggestionUtils';
import * as ReportActionsUtils from '../../../../libs/ReportActionsUtils';
import canFocusInputOnScreenFocus from '../../../../libs/canFocusInputOnScreenFocus';
import SilentCommentUpdater from './SilentCommentUpdater';
Expand Down Expand Up @@ -129,10 +130,8 @@ function ComposerWithSuggestions({
// A flag to indicate whether the onScroll callback is likely triggered by a layout change (caused by text change) or not
const isScrollLikelyLayoutTriggered = useRef(false);

const hasEnoughSpaceForLargeSuggestion =
listHeight - composerHeight - (CONST.CHAT_FOOTER_SECONDARY_ROW_HEIGHT + 2 * CONST.CHAT_FOOTER_SECONDARY_ROW_PADDING) >
CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_VISIBLE_SUGGESTIONS_IN_CONTAINER * CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTION_ROW_HEIGHT +
CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTER_INNER_PADDING * 2;
const hasEnoughSpaceForLargeSuggestion = SuggestionUtils.spaceAvailableForLargeSuggestionMenu(listHeight, composerHeight);

const isAutoSuggestionPickerLarge = !isSmallScreenWidth || (isSmallScreenWidth && hasEnoughSpaceForLargeSuggestion);

/**
Expand Down