[$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?

• [x] Android: Native
• [x] Android: mWeb Chrome
• [x] iOS: Native
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] 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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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.

https://github.com/Expensify/App/assets/35566748/32da87b2-e139-457e-8aec-8f098ae8f7f1

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 https://github.com/Expensify/App/issues/39832#issuecomment-2043170656 looks good and works well.

🎀 👀 🎀 C+ Reviewed

[melvin-bot[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: https://github.com/Expensify/App/issues/39832#issuecomment-2052312784

[abdulrahuman5196]

Hi @ShridharGoel , This issue is a feature request. The expectation would be to provide a solution to address the core issue by providing adequate information on how you would be solving the problem in PR. We don't expect code perfect sample in proposal, which would instead be done in the PR. Minor issues or changes would be handled as part of the PR process. So here the proposal which came first was suggested by me.

[nkdengineer]

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

@ShridharGoel This is a trivial issue and can be easily avoided with the use of memoization (like useCallback). I noticed already when proposing because it's even producing a react/no-unstable-nested-components lint issue. So there's no chance we'll miss it in the PR phase.

I didn't elaborate more on memoization when proposing because that was meant as a pseudo-code, not full fledged PR ready to merge as @abdulrahuman5196 mentioned

We don't expect code perfect sample in proposal, which would instead be done in the PR. Minor issues or changes would be handled as part of the PR process.

@arosiclair I think we can move forward here 👍

[dylanexpensify]

bump @arosiclair

[arosiclair]

@nkdengineer's proposal LGTM

Instead of having a separate ButtonWithDropdown component, we can add a prop to ButtonWithDropdownMenu to control this button style.

Yeah I think we should do this with an optional isSplit prop (true by default). That way we don't have to maintain 2 dropdown buttons separately.

[melvin-bot[bot]]

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

Offer link Upwork job

[melvin-bot[bot]]

📣 @nkdengineer You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

[arosiclair]

Oh I see @luacmartins self assigned. Lemme know if you still want to manage otherwise I can handle it 👍

[luacmartins]

I'll review the PR as well, since it touches many of the Simplified Collect pages I was involved with and I wanna make sure the logic still works as intended.

[dylanexpensify]

@abdulrahuman5196 @luacmartins mind giving an update?

[abdulrahuman5196]

Hi, I am going through PR review. Will provide update in couple of hours.

[luacmartins]

PR is merged

[luacmartins]

@Expensify/design could you please review the new split button styles on main? We merged the PR above with the logic for the new button, we'll address any style tweaks in a follow up if needed.

[shawnborton]

Is it deployed, or what is the best way to test it?