-
Notifications
You must be signed in to change notification settings - Fork 3k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a complicated way to say relevantMenuItems
?
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.
What does that mean? The name is not good or the logic is complicated?
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 meant that...
_.map(whatever, (x) => x)
should always be equivalent to whatever
, unless I'm missing something
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/VideosMacOS: 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, outdated name. Also a few lines later. I missed it before.
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.
@cubuspl42 Thanks, updated name of variable.
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.
Looks great other than a couple small things I would like to see fixed. I particularly find some of the naming misleading.
Please DM me on NewDot when you get it updated and I can review promptly.
error={shouldDisplayFieldError && TransactionUtils.isAmountMissing(transaction) ? translate('common.error.enterAmount') : ''} | ||
/> | ||
), | ||
isRelevant: true, |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dukenv0307 This is a good observation! I agree that shouldShow
is a suitable name here.
@cubuspl42 @neil-marcellini I updated these suggestions above. |
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.
Looks really good now, thanks so much!
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...instead of this one here.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's put the more explanatory comment here please
|
@cubuspl42 I resolved the conflict and moved the comment to the correct place. |
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.
Great, thank you!
🚀 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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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