Skip to content
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

[Due for payment 2025-02-18] [$250] Add distance label on map route #53421

Open
dubielzyk-expensify opened this issue Dec 3, 2024 · 95 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Monthly KSv2

Comments

@dubielzyk-expensify
Copy link
Contributor

dubielzyk-expensify commented Dec 3, 2024

Problem

Today when users submit a distance expense there's no way for users to see how long the distance is without continuing.

Solution

To improve quality of life and bring clarity to the distance request, let's add a distance label on the map route to ensure users can see the distance before they go to the detail screen.

Image

I believe the Mapbox API allows us to do this pretty easily but I'm open to styling it differently depending on what controls we have.

  • If the route contains multiple stops, we should not do individual legs, but rather only the distance of the full route.
  • If your default workspace is the personal workspace, we'd display the unit associated with the P2P rate/unit we've assigned you from the currencyToDefaultMileageRate list based on the output currency of the personal workspace.
  • If you tap/click on the tooltip it changes from miles/km, so everyone has the chance to see both if they want (i.e travelling abroad or something).

cc @Expensify/design

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021864707242087404252
  • Upwork Job ID: 1864707242087404252
  • Last Price Increase: 2024-12-26
  • Automatic offers:
    • shubham1206agra | Contributor | 105535036
    • truph01 | Contributor | 105692875
Issue OwnerCurrent Issue Owner: @shubham1206agra
@dubielzyk-expensify dubielzyk-expensify added NewFeature Something to build that is a new item. Improvement Item broken or needs improvement. Engineering and removed NewFeature Something to build that is a new item. labels Dec 3, 2024
Copy link

melvin-bot bot commented Dec 3, 2024

Triggered auto assignment to @bfitzexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

Copy link

melvin-bot bot commented Dec 3, 2024

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

Copy link

melvin-bot bot commented Dec 3, 2024

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

Copy link

melvin-bot bot commented Dec 3, 2024

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

@truph01
Copy link
Contributor

truph01 commented Dec 4, 2024

Proposal

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

  • Add distance label on map route

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 create a custom symbol component based on design. This component will get the distance data from transaction (via TransactionUtils.getDistanceInMeters(transaction, 'km')), and then display that distance data in here, like we did when display the current position marker.
  1. Get the distance data.
  • As mentioned above, the distance data can be get from TransactionUtils.getDistanceInMeters(transaction)).
  1. Get the coordinate to display the distance label.
  • The coordinate of the distance label can be the middle element in the directionCoordinates. So in here, we can add:
        const distanceSymbolCoorinate = useMemo(() => {
            const length = directionCoordinates?.length;
            // If the array is empty, return undefined
            if (!length) return undefined;

            // Find the index of the middle element
            const middleIndex = Math.floor(length / 2);

            // Return the middle element
            return directionCoordinates.at(middleIndex);
        }, [directionCoordinates]);
  1. Display the distance label.
  • In here, use the Marker to display the distance label:
                     {!!distanceSymbolCoorinate && !!distance && (
                        <Marker
                            key="distance"
                            longitude={distanceSymbolCoorinate.at(0) ?? 0}
                            latitude={distanceSymbolCoorinate.at(1) ?? 0}
                        >
                            <View style={{marginRight: 100}}>
                                <View
                                    style={{backgroundColor: '#60d184', paddingHorizontal: 10, paddingVertical: 4, color: 'white', borderRadius: 12, fontWeight: 'bold', textAlign: 'center'}}
                                >
                                    <View style={{fontSize: 16}}> {DistanceRequestUtils.getDistanceForDisplayLabel(distance, CONST.CUSTOM_UNITS.DISTANCE_UNIT_KILOMETERS)}</View>
                                    <View style={{fontSize: 12}}> {DistanceRequestUtils.getDistanceForDisplayLabel(distance, CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES)}</View>
                                </View>
                            </View>
                        </Marker>
                    )}

It is just the mock style. The detailed should be address when we implement in PR.

  1. The similar can be applied in native.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 4, 2024
@bfitzexpensify bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label Dec 5, 2024
@melvin-bot melvin-bot bot changed the title Add distance label on map route [$250] Add distance label on map route Dec 5, 2024
Copy link

melvin-bot bot commented Dec 5, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 5, 2024
Copy link

melvin-bot bot commented Dec 5, 2024

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

Copy link

melvin-bot bot commented Dec 9, 2024

@bfitzexpensify, @thesahindia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Dec 9, 2024
@dubielzyk-expensify
Copy link
Contributor Author

Sorry @JmillsExpensify just saw your concern about whether to use miles or km. Do we wanna do something like this instead? I assume adding metric/imperial preference in profile is too much at this point?

CleanShot 2024-12-10 at 13 39 35@2x

cc @Expensify/design

@shawnborton
Copy link
Contributor

I quite like your middle treatment there.

Is there a way to just use someone's primary currency/location to determine whether we initially show km or mi? Most of our customers are US-based and don't need to worry about km so I don't want to over solve this problem, so-to-speak.

@dannymcclain
Copy link
Contributor

I quite like your middle treatment there.
Is there a way to just use someone's primary currency/location to determine whether we initially show km or mi? Most of our customers are US-based and don't need to worry about km so I don't want to over solve this problem, so-to-speak.

Big agree with all of this. Love the middle treatment. I think we should either make an educated guess about which to make the primary unit, or just always lead with miles and let kilometers be the secondary for now (since most of our customers are US-based).

@dubielzyk-expensify
Copy link
Contributor Author

I'll add the question to the original post and see if there's a way we can use timezone to detect 👍 Let's use the middle one

Copy link

melvin-bot bot commented Dec 11, 2024

@bfitzexpensify, @thesahindia Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Dec 12, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Dec 13, 2024

@bfitzexpensify, @thesahindia Still overdue 6 days?! Let's take care of this!

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Feb 11, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.95-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-02-18. 🎊

For reference, here are some details about the assignees on this issue:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 17, 2025
@bfitzexpensify
Copy link
Contributor

@shubham1206agra please complete the BZ checklist

{ayment complete to @truph01

@melvin-bot melvin-bot bot added the Overdue label Feb 21, 2025
@bfitzexpensify
Copy link
Contributor

Bump @shubham1206agra

@melvin-bot melvin-bot bot removed the Overdue label Feb 21, 2025
@shubham1206agra
Copy link
Contributor

New feature so only tests are required.

And for tests, we do not require additional tests as prior/existing tests will cover this issue too.

@melvin-bot melvin-bot bot added the Overdue label Feb 24, 2025
@bfitzexpensify
Copy link
Contributor

Thanks @parasharrajat - looks like both issues tagged the offending PR but not this issue, which is why I didn't see them linked.

OK, I've adjusted the payment down to $62.50. Offer resent @shubham1206agra

@melvin-bot melvin-bot bot removed the Overdue label Feb 24, 2025
@shubham1206agra
Copy link
Contributor

@bfitzexpensify I am sorry, but the payment should be adjusted to $125 here.

@bfitzexpensify
Copy link
Contributor

@shubham1206agra pay is reduced by 50% for each regression. There are 2 regressions here, so the payment goes from $250 > $125 > $62.50.

@shubham1206agra
Copy link
Contributor

@bfitzexpensify For #56531, see #56531 (comment) as this was marked as minor UI issue (and was an edge case).

@bfitzexpensify
Copy link
Contributor

Not a blocker, but still a regression that had to be addressed.

@truph01
Copy link
Contributor

truph01 commented Feb 24, 2025

@bfitzexpensify Regarding issue #56531, we decided to skip that bug when implementing the PR, as mentioned in this comment. The issue was created as an improvement rather than a bug fix, so I don’t believe it should be considered a regression from the original PR.

@bfitzexpensify
Copy link
Contributor

Thanks for the context @truph01, I agree given that that we can ignore #56531 as a regression.

@bfitzexpensify
Copy link
Contributor

I updated the offer @shubham1206agra - offer should be in your Upwork account

Copy link

melvin-bot bot commented Mar 4, 2025

@bfitzexpensify Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Mar 4, 2025
@bfitzexpensify bfitzexpensify added the Weekly KSv2 label Mar 4, 2025
@melvin-bot melvin-bot bot removed the Overdue label Mar 4, 2025
@bfitzexpensify bfitzexpensify removed the Daily KSv2 label Mar 4, 2025
@bfitzexpensify
Copy link
Contributor

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Mar 28, 2025
Copy link

melvin-bot bot commented Mar 28, 2025

This issue has not been updated in over 15 days. @Gonals, @bfitzexpensify, @shubham1206agra, @truph01 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Monthly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants