Display require field above show more button #35287
Conversation
|
@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| {isCategoryRequired && ( | ||
| <MenuItemWithTopDescription | ||
| shouldShowRightIcon={!isReadOnly} | ||
| title={iouCategory} | ||
| description={translate('common.category')} | ||
| numberOfLinesTitle={2} | ||
| onPress={() => Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CATEGORY.getRoute(iouType, transaction.transactionID, reportID, Navigation.getActiveRouteWithoutParams()))} | ||
| style={[styles.moneyRequestMenuItem]} | ||
| titleStyle={styles.flex1} | ||
| disabled={didConfirm} | ||
| interactive={!isReadOnly} | ||
| rightLabel={translate('common.required')} | ||
| /> | ||
| )} | ||
| {isTagRequired && ( | ||
| <MenuItemWithTopDescription | ||
| shouldShowRightIcon={!isReadOnly} | ||
| title={PolicyUtils.getCleanedTagName(iouTag)} | ||
| description={policyTagListName} | ||
| numberOfLinesTitle={2} | ||
| onPress={() => Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_TAG.getRoute(iouType, transaction.transactionID, reportID, Navigation.getActiveRouteWithoutParams()))} | ||
| style={[styles.moneyRequestMenuItem]} | ||
| disabled={didConfirm} | ||
| interactive={!isReadOnly} | ||
| rightLabel={translate('common.required')} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Would you refactor this to reduce code duplication?
I'm thinking of something like this:
const classifiedMenuItems = [
shouldShowSmartScanFields && {
item: <MenuItemWithTopDescription
shouldShowRightIcon={!isReadOnly && !isDistanceRequest}
title={formattedAmount}
description={translate('iou.amount')}
interactive={!isReadOnly}
onPress={() => {
if (isDistanceRequest) {
return;
}
if (isEditingSplitBill) {
Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(reportID, reportActionID, CONST.EDIT_REQUEST_FIELD.AMOUNT));
return;
}
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_AMOUNT.getRoute(iouType, transaction.transactionID, reportID, Navigation.getActiveRouteWithoutParams()));
}}
style={[styles.moneyRequestMenuItem, styles.mt2]}
titleStyle={styles.moneyRequestConfirmationAmount}
disabled={didConfirm}
brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isAmountMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
error={shouldDisplayFieldError && TransactionUtils.isAmountMissing(transaction) ? translate('common.error.enterAmount') : ''}
/>,
isRelevant: true,
},
{
item: <MenuItemWithTopDescription
shouldShowRightIcon={!isReadOnly}
shouldParseTitle
title={iouComment}
description={translate('common.description')}
onPress={() => {
if (isEditingSplitBill) {
Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(reportID, reportActionID, CONST.EDIT_REQUEST_FIELD.DESCRIPTION));
return;
}
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_DESCRIPTION.getRoute(iouType, transaction.transactionID, reportID, Navigation.getActiveRouteWithoutParams()));
}}
style={[styles.moneyRequestMenuItem]}
titleStyle={styles.flex1}
disabled={didConfirm}
interactive={!isReadOnly}
numberOfLinesTitle={2}
/>,
isRelevant: true,
},
{
item: <MenuItemWithTopDescription
shouldShowRightIcon={!isReadOnly}
title={isMerchantEmpty ? '' : iouMerchant}
description={translate('common.merchant')}
style={[styles.moneyRequestMenuItem]}
titleStyle={styles.flex1}
onPress={() => {
if (isEditingSplitBill) {
Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(reportID, reportActionID, CONST.EDIT_REQUEST_FIELD.MERCHANT));
return;
}
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_MERCHANT.getRoute(iouType, transaction.transactionID, reportID, Navigation.getActiveRouteWithoutParams()));
}}
disabled={didConfirm}
interactive={!isReadOnly}
brickRoadIndicator={merchantError ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
error={merchantError ? translate('common.error.fieldRequired') : ''}
/>,
isRelevant: isMerchantRequired,
},
// ...
];
const relevantMenuItems = classifiedMenuItems
.filter((classifiedMenuItem) => classifiedMenuItem.isRelevant)
.map((classifiedMenuItem) => classifiedMenuItem.item);
const supplementaryMenuItems = classifiedMenuItems
.filter((classifiedMenuItem) => !classifiedMenuItem.isRelevant)
.map((classifiedMenuItem) => classifiedMenuItem.item);We would show relevantMenuItems above the "Show more" pill and the supplementaryMenuItems below that pill.
There was a problem hiding this comment.
That's great. Updating now.
|
@cubuspl42 I updated to refactor render menu item. |
| error={merchantError ? translate('common.error.fieldRequired') : ''} | ||
| /> | ||
| )} | ||
| {_.map(relevantMenuItems, (relevantMenuItem) => relevantMenuItem)} |
There was a problem hiding this comment.
Isn't this a complicated way to say relevantMenuItems?
There was a problem hiding this comment.
What does that mean? The name is not good or the logic is complicated?
There was a problem hiding this comment.
I meant that...
_.map(whatever, (x) => x)should always be equivalent to whatever, unless I'm missing something
There was a problem hiding this comment.
You're right. I updated.
| <View | ||
| style={[styles.flexRow, styles.justifyContentBetween, styles.alignItemsCenter, styles.ml5, styles.mr8, styles.optionRow]} | ||
| key={translate('common.billable')} | ||
| > | ||
| <Text color={!iouIsBillable ? theme.textSupporting : undefined}>{translate('common.billable')}</Text> | ||
| <Switch | ||
| accessibilityLabel={translate('common.billable')} | ||
| isOn={iouIsBillable} | ||
| onToggle={onToggleBillable} | ||
| /> | ||
| </View> |
There was a problem hiding this comment.
Ouch, it's not a MenuItem
Maybe let's switch the naming to "fields"? We have a local variable like shouldShowAllFields, so we can be consistent with that, abstracting from specific component names like MenuItem.
So classifiedFields, relevantFields, etc...
Minor formatting issue As per "Tests": please always mention the betas that need to be enabled. |
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop |
|
@cubuspl42 I updated formatting and tests. |
I know it's a minor thing, but it's also very easy to fix. |
|
We have bad luck; there's another conflict. Please resolve it. I'll perform a final re-test then, and we'll be good to merge. |
|
@cubuspl42 I resolved the conflict. |
|
|
||
| const relevantFields = _.map( | ||
| _.filter(classifiedFields, (classifiedField) => !classifiedField || classifiedField.isRelevant), | ||
| (relevantMenuItem) => relevantMenuItem.item, |
There was a problem hiding this comment.
Oops, outdated name. Also a few lines later. I missed it before.
There was a problem hiding this comment.
@cubuspl42 Thanks, updated name of variable.
| error={shouldDisplayFieldError && TransactionUtils.isAmountMissing(transaction) ? translate('common.error.enterAmount') : ''} | ||
| /> | ||
| ), | ||
| isRelevant: true, |
There was a problem hiding this comment.
I don't like the name isRelevant. It makes me think that it won't be rendered if it's not relevant. I know that we're trying to determine if it should display above the "Show more" button. What about inverting the logic and going with this name instead isSupplementary? I think that's more clear. Or alternatively isPrimaryField.
Then below we can render primaryFields and supplementaryFields.
There was a problem hiding this comment.
I assumed that it is fine to tell that fields aren't much relevant if we hide them by default, but I don't have strong opinions here.
I think that I would choose flipping the logic and going with isSupplementary / primaryFields / supplementaryFields
| ]; | ||
|
|
||
| const relevantFields = _.map( | ||
| _.filter(classifiedFields, (classifiedField) => !classifiedField || classifiedField.isRelevant), |
There was a problem hiding this comment.
Why do we have the condition !classifiedField? I don't think we will have any falsey objects in the list of fields, and even if we did, how would that make it "relevant"?
There was a problem hiding this comment.
This is a great catch and I missed this change. It's actually my fault, as I kind of naively suggested porting the React conventions to "pure" JavaScript, without thinking much of how this will work out.
The root cause is the expression shouldShowSmartScanFields && { ... }, which evaluates to false if shouldShowSmartScanFields is false. This is indeed smelly.
Maybe we could go with...
const classifiedFields = _.filter([
shouldShowSmartScanFields ? { /* stuff */ } : undefined,
// more stuff
], classifiedField !== undefined);What do you guys think?
There was a problem hiding this comment.
Ah ok I see, makes sense. I actually missed the fact that some elements would be false, because it's easy to miss that this is "pure" JS.
I think your suggestion is good. I might suggest setting a shouldRender field in the item object, instead of using the ternary expression, then filtering to include only items that shouldRender. I think that's slightly more clear. Either way it will be better then its current form.
There was a problem hiding this comment.
shouldRender
That's sound good to me, maybe we can called this like shouldShow since the variable to check this also started with shouldShow.
There was a problem hiding this comment.
@dukenv0307 This is a good observation! I agree that shouldShow is a suitable name here.
|
@cubuspl42 @neil-marcellini I updated these suggestions above. |
|
@dukenv0307 looks like there are some conflicts. Please let me know when they are resolved and I can get this merged for you. |
|
Maybe we could also resolve this last thing? |
|
Conflicts and one last thread to close. Maybe also merge |
|
@neil-marcellini @cubuspl42 I resolved conflict and add a comment. |
| // An intermediate structure that helps us classify the fields as "primary" and "supplementary". | ||
| // The primary fields are always shown to the user, while an extra action is needed to reveal the supplementary ones. |
There was a problem hiding this comment.
Sorry, there was a misunderstanding. I think we meant this comment to go... 👉 #35287 (comment)
| @@ -624,6 +627,233 @@ function MoneyTemporaryForRefactorRequestConfirmationList({ | |||
| ); | |||
| }, [isReadOnly, iouType, selectedParticipants.length, confirm, bankAccountRoute, iouCurrencyCode, policyID, splitOrRequestOptions, formError, styles.ph1, styles.mb2]); | |||
|
|
|||
| // All fields that a request can have | |||
There was a problem hiding this comment.
...instead of this one here.
There was a problem hiding this comment.
The intention was to:
- make the code easier to grasp in general
- explain the "classified" part of the local variable name
There was a problem hiding this comment.
Yeah, let's put the more explanatory comment here please
|
|
@cubuspl42 I resolved the conflict and moved the comment to the correct place. |
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.41-12 🚀
|
Details
Display required field above show more button
Fixed Issues
$ #34751
PROPOSAL: #34751 (comment)
Tests
Precondition: have a workspace with the tag and category are required and enable violation beta
Offline tests
Same as above
QA Steps
Precondition: have a workspace with the tag and category are required and enable violation beta
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2024-01-28.at.20.53.02.mov
Android: mWeb Chrome
Screen.Recording.2024-01-28.at.20.50.52.mov
iOS: Native
iOS: mWeb Safari
Screen.Recording.2024-01-28.at.20.48.23.mov
MacOS: Chrome / Safari
Screen.Recording.2024-01-28.at.20.41.47.mov
MacOS: Desktop
Screen.Recording.2024-01-28.at.20.58.28.mov