[$250] [Search v2.3] Popup colors are incorrect

[lakchote]

See #48566 (comment)

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021836682888839715103
• Upwork Job ID: 1836682888839715103
• Last Price Increase: 2024-09-19
• Automatic offers:
• ishpaul777 | Contributor | 104052106
• jaydamani | Contributor | 104052748

Issue Owner

Current Issue Owner: @mollfpr

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~021836682888839715103

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

[cretadn22]

@lakchote Could you provide the correct design?

[jaydamani]

Proposal

Please 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 theme.icon (green) and color on focus is theme.iconMenu (black). The colors are decided based on below logic (active means hovered).

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 1

Only update the menu for saved search.

To do this, add isPaneMenu as prop to ThreeDotMenu and popOverMenu to pass it down to MenuItem

Option 2

Update 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;
    }

[NaveedShaikh78]

Proposal

Please re-state the problem that we are trying to solve in this issue

Popup 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
done revision

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

[melvin-bot]

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[lakchote]

Assigning @ishpaul777 since he's available to review now.

[ishpaul777]

Thanks, looking at proposals 👀

[ishpaul777]

@lakchote is this the correct colors ?

Screen.Recording.2024-09-20.at.2.31.15.PM.mov

asking 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

[ishpaul777]

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

[melvin-bot]

Current assignee @lakchote is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[lakchote]

Let's go with option 1 @jaydamani as we don't want to introduce unwanted side effects in the app, as isPaneMenu must have been there for a reason.

[melvin-bot]

📣 @jaydamani 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[jaydamani]

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.

[lakchote]

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!

[jaydamani]

Created PR here

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker 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.

[jaydamani]

This was deployed to prod and the review period is over.

[jaydamani]

@ishpaul777 @lakchote Payment is due here but this is not assigned to BZ team so, can you please help out here as I am not sure who to ping. Thanks for the help.

[melvin-bot]

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lakchote]

@ishpaul777 @lakchote Payment is due here but this is not assigned to BZ team so, can you please help out here as I am not sure who to ping. Thanks for the help.

Done!

[joekaufmanexpensify]

Happy to help with payment here! I see on the PR there was a regression, so will factor that into payment. We need to pay:

• @jaydamani - $125 for PR via Upwork (50% regression penalty)
• @ishpaul777 - $125 for C+ via Upwork (50% regression penalty)

[joekaufmanexpensify]

@jaydamani $125 sent and contract ended!

[joekaufmanexpensify]

@ishpaul777 $125 sent and contract ended!

[joekaufmanexpensify]

Upwork job closed

[joekaufmanexpensify]

All set, thanks everyone!

[ishpaul777]

hey, i think the linked issue was not regression but expected behaviour so we closed it without action, there was another very minor issue #49885 which was resolved by me same day its found, the PR touched the whole app icon colors so it was expected to found some minor issues fortunately there was only one. Can we please rethink the applied penalty, cc @lakchote @joekaufmanexpensify @luacmartins

[joekaufmanexpensify]

Yeah, that second one is the one I was referring to, as it was called out as a regression on the PR , and then immediately fixed. Typically, any regression does trigger the penalty, but I am coming into this one late, so happy to defer to @lakchote or @luacmartins if they feel differently.

[luacmartins]

I understand the situation, but I think we should stick to the rule on this one.