[HOLD for payment 2024-05-08] [$250] Update "X selected" button above table views to be a dropdown button instead of a split button

[shawnborton]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v1.4.61-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): N/A
Logs: https://stackoverflow.com/c/expensify/questions/4856 N/A
Expensify/Expensify Issue URL: N/A
Issue reported by: @dannymcclain
Slack conversation: Link

Action Performed:

1. Go to any part of the new workspace editor for collect policies that has a table view. For example: tags, taxes, categories, members, or distance rates.
2. Select rows of data using the checkboxes on the left side of the row

Expected Result:

1. Rather than using a split button, we should be using a dropdown button. A dropdown button just has a small arrow right after the label text and does not have two distinct "zones" in the button:

Actual Result:

1. We're currently using a split button, which doesn't make sense for this UI interaction and also prevents the user from using the larger part of the button.

Workaround:

The smaller arrow part of the split button still works just fine.

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

View all open jobs on GitHub

cc @Expensify/design @mountiny @luacmartins @trjExpensify

Per Slack, let's start this one at $250.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017b3c66ee08aa3e77
• Upwork Job ID: 1777360525265432576
• Last Price Increase: 2024-04-08
• Automatic offers:
• abdulrahuman5196 | Reviewer | 0

Issue Owner

Current Issue Owner: @dylanexpensify

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

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

[nkdengineer]

Proposal

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

We're currently using a split button, which doesn't make sense for this UI interaction and also prevents the user from using the larger part of the button.

What is the root cause of that problem?

This is new feature change

What changes do you think we should make in order to solve the problem?

Add a prop to ButtonWithDropdownMenu to control this new button style, we can call it isSplit (true by default) as recommended here.
If isSplit is false, it will have the following differences:

• Remove the divider in between, by removing the whole Caret button here
• Make the full button width clickable, by using this Button and make the caret the iconRight

iconRight={() => <Icon
    medium={isButtonSizeLarge}
    small={!isButtonSizeLarge}
    src={Expensicons.DownArrow}
    fill={success ? theme.buttonSuccessText : theme.icon}
/>}
shouldShowRightIcon

(We'll of course put the () => <Icon in memoization like useCallback so it will not cause unnecessary rerendering)

And remove the customText !== undefined && styles.cursorDefault, customText !== undefined && styles.pointerEventsNone here

• Make sure the PopoverMenu here has the main Button above as the anchor, by setting the caretButton to the main button ref. In here:

ref={(ref) => {
    // we need to add logic to preserve the buttonRef
    caretButton.current = ref;
}}

These are all straight forward style changes

And use the isSplit false anywhere we need the ButtonWithDropdown with the above style.

What alternative solutions did you explore? (Optional)

NA

Result

[nexarvo]

Proposal

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

Update "X selected" button above table views to be a dropdown button instead of a split button

What is the root cause of that problem?

New Feature

What changes do you think we should make in order to solve the problem?

We need to update the ButtonWithDropDownMenu to show the drop down without any separator. Right now we only have the option to show drop down with separator. We can take a props as showWithSeparator.
And if shouldAlwaysShowDropdownMenu is true we will also check if showWithSeparator is true and the show the the separator otherwise will show dropdown button without separator.

To add showWithSeparator props is needed because drop down button with separator is still used at many places so we want to make sure it also work as expected.

Pseudo Code

const renderButton = () => {
  if (shouldAlwaysShowDropdownMenu) {
    if (showWithSeparator) {
      // We will show drop down button with separator here
    } else {
      // We will show drop down button without separator here
      <Button
          success={success}
          ref={caretButton}
          pressOnEnter={pressOnEnter}
          isDisabled={isDisabled || !!options[0].disabled}
          style={[styles.w100, style]}
          isLoading={isLoading}
          text={customText ?? selectedItem.text}
          onPress={() => setIsMenuVisible(!isMenuVisible)}
          large={isButtonSizeLarge}
          medium={!isButtonSizeLarge}
          innerStyles={[innerStyleDropButton]}
          enterKeyEventListenerPriority={enterKeyEventListenerPriority}
          shouldShowRightIcon={true}
          iconRight={Expensicons.DownArrow}
      />
    }
  } else {
    // We will how simple button here
  }
};

What alternative solutions did you explore? (Optional)

Result

Note

We will need to adjust inner style of this new button.

[ShridharGoel]

Proposal

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

We need to combine text and dropdown icon for dropdowns buttons.

What is the root cause of that problem?

New change.

What changes do you think we should make in order to solve the problem?

Add a new prop in ButtonWithDropdownMenu called as shouldCombineTextAndIcon or something similar, which should be passed as true when we want to combine the text and dropdown.

Define caretIcon like this: (Why? - Because this component shouldn't be added directly inside iconRight since it can lead to performance issues)

const caretIcon = () => (
    <Icon
        medium={isButtonSizeLarge}
        small={!isButtonSizeLarge}
        src={Expensicons.DownArrow}
        fill={success ? theme.buttonSuccessText : theme.icon}
    />
);

If shouldCombineTextAndIcon is true, then instead of this button, the below needs to be shown:

<Button
    success={success}
    pressOnEnter={pressOnEnter}
    ref={caretButton}
    onPress={() => setIsMenuVisible(!isMenuVisible)}
    text={customText ?? selectedItem.text}
    isDisabled={isDisabled || !!selectedItem.disabled}
    isLoading={isLoading}
    style={[styles.flex1, styles.pr10, styles.pl0]}
    large={isButtonSizeLarge}
    medium={!isButtonSizeLarge}
    innerStyles={innerStyleDropButton}
    enterKeyEventListenerPriority={enterKeyEventListenerPriority}
    iconRight={caretIcon}
    shouldShowRightIcon
/>

Also, this would be omitted if shouldCombineTextAndIcon is true.

Screen.Recording.2024-04-08.at.10.47.16.PM.mov

I can provide a test branch if needed.

[dannymcclain]

Just a quick note that the updated button doesn't need to be a fixed width. It can just can just follow the standard button padding.

Reference here for medium buttons with a right icon (since I think this is a medium button?):

cc @Expensify/design for any dissenting opinions.

[ShridharGoel]

Proposal

Updated to remove extra padding. Now, it will look like medium button.

[shawnborton]

I think it's a medium button, yup. And good shout Danny, I agree it doesn't need to be fixed.

[nkdengineer]

Proposal updated to remove the additional padding, based on design team's confirmation

[dylanexpensify]

Bump @abdulrahuman5196 to review

[abdulrahuman5196]

Checking now

[abdulrahuman5196]

This is a straightforward feature request. So choosing the first proposal with adequate information to work on the implementation.

@nkdengineer 's proposal here #39832 (comment) looks good and works well.

🎀 👀 🎀
C+ Reviewed

[melvin-bot]

Triggered auto assignment to @arosiclair, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[ShridharGoel]

The selected proposal mentions this:

iconRight={() => <Icon
    medium={isButtonSizeLarge}
    small={!isButtonSizeLarge}
    src={Expensicons.DownArrow}
    fill={success ? theme.buttonSuccessText : theme.icon}
/>}

This would lead to performance issues because of unnecessary re-renders.

I've mentioned the correct way here.

So, can you check the proposals again if possible?

[dylanexpensify]

@luacmartins or @arosiclair to confirm proposal req from Abdul!

[ShridharGoel]

@luacmartins @arosiclair Kindly check this comment before finalising: #39832 (comment)

[dylanexpensify]

bump @abdulrahuman5196 on checklist

[dylanexpensify]

@nkdengineer please apply at the job! 🙇‍♂️

[dylanexpensify]

bump @nkdengineer

[nkdengineer]

Hi @dylanexpensify my Upwork account doesn't have any Connects left so I'm unable to apply. Could you directly send an offer/invitation to my Upwork profile?

Thanks 🙇

[melvin-bot]

@arosiclair, @luacmartins, @abdulrahuman5196, @dylanexpensify, @nkdengineer Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@arosiclair, @luacmartins, @abdulrahuman5196, @dylanexpensify, @nkdengineer 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[dylanexpensify]

sent invite @nkdengineer!

[abdulrahuman5196]

The PR that introduced the bug has been identified. Link to the PR:
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

Not a regression. New feature.

Determine if we should create a regression test for this bug.
If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

1. Go to workspace setting
2. Enable all features: distance rates, categories, tags, taxes
3. Select rows of data using the checkboxes on the left side of the row
4. Verify that: A dropdown button with no split is displayed in top right
5. Click to this button
6. Verify that: A dropdown is working fine

@dylanexpensify

[nkdengineer]

sent invite @nkdengineer!

@dylanexpensify I applied to the job!

[melvin-bot]

@arosiclair, @luacmartins, @abdulrahuman5196, @dylanexpensify, @nkdengineer Huh... This is 4 days overdue. Who can take care of this?

[dylanexpensify]

@nkdengineer offer sent!

[nkdengineer]

@dylanexpensify Thank you 🙇 I accepted the offer

[melvin-bot]

@arosiclair, @luacmartins, @abdulrahuman5196, @dylanexpensify, @nkdengineer Eep! 4 days overdue now. Issues have feelings too...

[luacmartins]

Are we good to close this one?

[abdulrahuman5196]

Are we good to close this one?

I got paid. If @nkdengineer is also paid we can close this? @dylanexpensify

[nkdengineer]

Sorry @dylanexpensify it seems I wasn't paid here yet, could you reopen the issue to help with this?

TIA

[dylanexpensify]

Yes!

[dylanexpensify]

Done!