-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[$250] [Search v2.3] Popup colors are incorrect #49440
Comments
Job added to Upwork: https://www.upwork.com/jobs/~021836682888839715103 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr ( |
@lakchote Could you provide the correct design? |
ProposalPlease re-state the problem that we are trying to solve in this issue.Incorrect color in three dot menu What is the root cause of that problem?The popup comes from ThreeDotMenu which renders popOverMenu. PopOverMenu does not pass isPaneMenu prop for MenuItem so the default icon color is https://github.com/Expensify/App/blob/main/src/styles/utils/index.ts#L1442-L1460 What changes do you think we should make in order to solve the problem?The colors come from common components so we can do one of the following: Option 1Only update the menu for saved search. To do this, add isPaneMenu as prop to ThreeDotMenu and popOverMenu to pass it down to MenuItem Option 2Update all pop-up menus in the app to be consitent. Add isPaneMenu prop here or remove the logic from here by updating to below. We can also remove isPaneMenu prop after this. switch (buttonState) {
case CONST.BUTTON_STATES.ACTIVE:
case CONST.BUTTON_STATES.PRESSED:
return theme.iconHovered;
case CONST.BUTTON_STATES.COMPLETE:
return theme.iconSuccessFill;
case CONST.BUTTON_STATES.DEFAULT:
case CONST.BUTTON_STATES.DISABLED:
default:
if (isMenuIcon) {
return theme.iconMenu;
}
return theme.icon;
} |
ProposalPlease re-state the problem that we are trying to solve in this issuePopup colors are incorrect What is the root cause of that problem?The deault value for menu item is wrong and but if we change the value it is not reflecting due to the changes What changes do you think we should make in order to solve the problem?MenuItem.tsx mak isPaneMenu flag to true What alternative solutions did you explore? (Optional)Reverting the changes revision. can fix the problem |
📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Assigning @ishpaul777 since he's available to review now. |
Thanks, looking at proposals 👀 |
@lakchote is this the correct colors ? Screen.Recording.2024-09-20.at.2.31.15.PM.movasking coz all other menu in app is of same color as now do we need to make all consistent with this colors Screen.Recording.2024-09-20.at.2.33.55.PM.mov |
if we want to make this change to all popover menu items in app, we can go with option 2 from @jaydamani's Proposal if only specific to saved Search popover menu then option 1. In any case @jaydamani Proposal LGTM! 🎀 👀 🎀 C+ reviewed |
Current assignee @lakchote is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
Let's go with option 1 @jaydamani as we don't want to introduce unwanted side effects in the app, as |
📣 @jaydamani 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
I received the offer. I will work on this over the weekend and raise PR by monday. Let me know if this needs to be done today. |
@jaydamani if you could work on this today and raise a PR, that'd be great! |
Created PR here |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
See #48566 (comment)
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @mollfprThe text was updated successfully, but these errors were encountered: