-
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
cleanup: moved report field options to their own file #52311
Merged
dangrous
merged 1 commit into
Expensify:main
from
margelo:cleanup/optionlistutils-report-field
Nov 12, 2024
Merged
Changes from all commits
Commits
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| import * as Localize from './Localize'; | ||
| import type {Option} from './OptionsListUtils'; | ||
| import type * as ReportUtils from './ReportUtils'; | ||
|
|
||
| /** | ||
| * Transforms the provided report field options into option objects. | ||
| * | ||
| * @param reportFieldOptions - an initial report field options array | ||
| */ | ||
| function getReportFieldOptions(reportFieldOptions: string[]): Option[] { | ||
| return reportFieldOptions.map((name) => ({ | ||
| text: name, | ||
| keyForList: name, | ||
| searchText: name, | ||
| tooltipText: name, | ||
| isDisabled: false, | ||
| })); | ||
| } | ||
|
|
||
| /** | ||
| * Build the section list for report field options | ||
| */ | ||
| function getReportFieldOptionsSection({ | ||
| options, | ||
| recentlyUsedOptions, | ||
| selectedOptions, | ||
| searchValue, | ||
| }: { | ||
| options: string[]; | ||
| recentlyUsedOptions: string[]; | ||
| selectedOptions: Array<Partial<ReportUtils.OptionData>>; | ||
| searchValue: string; | ||
| }) { | ||
| const reportFieldOptionsSections = []; | ||
| const selectedOptionKeys = selectedOptions.map(({text, keyForList, name}) => text ?? keyForList ?? name ?? '').filter((o) => !!o); | ||
| let indexOffset = 0; | ||
|
|
||
| if (searchValue) { | ||
| const searchOptions = options.filter((option) => option.toLowerCase().includes(searchValue.toLowerCase())); | ||
|
|
||
| reportFieldOptionsSections.push({ | ||
| // "Search" section | ||
| title: '', | ||
| shouldShow: true, | ||
| indexOffset, | ||
| data: getReportFieldOptions(searchOptions), | ||
| }); | ||
|
|
||
| return reportFieldOptionsSections; | ||
| } | ||
|
|
||
| const filteredRecentlyUsedOptions = recentlyUsedOptions.filter((recentlyUsedOption) => !selectedOptionKeys.includes(recentlyUsedOption)); | ||
| const filteredOptions = options.filter((option) => !selectedOptionKeys.includes(option)); | ||
|
|
||
| if (selectedOptionKeys.length) { | ||
| reportFieldOptionsSections.push({ | ||
| // "Selected" section | ||
| title: '', | ||
| shouldShow: true, | ||
| indexOffset, | ||
| data: getReportFieldOptions(selectedOptionKeys), | ||
| }); | ||
|
|
||
| indexOffset += selectedOptionKeys.length; | ||
| } | ||
|
|
||
| if (filteredRecentlyUsedOptions.length > 0) { | ||
| reportFieldOptionsSections.push({ | ||
| // "Recent" section | ||
| title: Localize.translateLocal('common.recent'), | ||
| shouldShow: true, | ||
| indexOffset, | ||
| data: getReportFieldOptions(filteredRecentlyUsedOptions), | ||
| }); | ||
|
|
||
| indexOffset += filteredRecentlyUsedOptions.length; | ||
| } | ||
|
|
||
| reportFieldOptionsSections.push({ | ||
| // "All" section when items amount more than the threshold | ||
| title: Localize.translateLocal('common.all'), | ||
| shouldShow: true, | ||
| indexOffset, | ||
| data: getReportFieldOptions(filteredOptions), | ||
| }); | ||
|
|
||
| return reportFieldOptionsSections; | ||
| } | ||
|
|
||
| export {getReportFieldOptionsSection, getReportFieldOptions}; |
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.
NAB - Should we re-order these to match the params better, and not have to name them? I could go either way
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.
sorry not sure what you mean, you mean that we rename
validFieldOptionstooptionsso we don't have to specify the object property - or to not use an object for thegetReportFieldOptionsSectionbut separate parameters?I think we have no style guideline on that but as soon as a function takes more than three parameters I usually create an object type as single parameter. For me this helps with readability, understanding what other options you can pass without opening the function declaration (intelli sense) and avoid having to pass
undefinedas a parameter explicitly when when we want to skip passing a certain parameterThere 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.
yeah i think that's fine! I just meant that the order they're listed in the function definition is different than the order we're passing them here. But it's not a huge deal, no need to change