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

[HOLD for payment 2021-10-11] Use default OS date picker for date fields #5340

Closed
JmillsExpensify opened this issue Sep 17, 2021 · 21 comments
Closed
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 Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review Weekly KSv2

Comments

@JmillsExpensify
Copy link

JmillsExpensify commented Sep 17, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Problem

The keyboard does not "match" certain fields that should only include dates within the VBA flow, which requires users to tap through to numbers within their keyboard manually. Here is where this occurs:

Company Information Step

  • Incorporation Date

Personal Information Step

  • DOB

Action Performed:

  1. Create workspace
  2. Go through VBA flow with test credentials

Solution

Use the default OS date picker for date fields whenever a field within the VBA flow will only include dates, like so:
IMG_2398F886A335-1

Platform:

  • iOS
  • Android

Reproducible in staging?: Yes
Reproducible in production?: Yes

Notes/Photos/Videos:

We should use the date picker pad in scenarios like this one:

IMG_2398F886A335-1

Upwork issue: https://www.upwork.com/jobs/~0123ace70fab645970

View all open jobs on GitHub

@JmillsExpensify JmillsExpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Sep 18, 2021
@MelvinBot
Copy link

Triggered auto assignment to @jboniface (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed AutoAssignerTriage Auto assign issues for triage to an available triage team member labels Sep 18, 2021
@jboniface jboniface removed their assignment Sep 20, 2021
@MelvinBot
Copy link

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@francoisl francoisl added the External Added to denote the issue can be worked on by a contributor label Sep 20, 2021
@francoisl francoisl removed their assignment Sep 20, 2021
@MelvinBot
Copy link

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot removed the Overdue label Sep 20, 2021
@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors and removed Daily KSv2 labels Sep 21, 2021
@MelvinBot
Copy link

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

@trjExpensify
Copy link
Contributor

trjExpensify commented Sep 21, 2021

Upwork issue here: https://www.upwork.com/jobs/~0123ace70fab645970

CC: @MitchExpensify @kevinksullivan as it's N6 polish.

@UHaider
Copy link

UHaider commented Sep 21, 2021

Proposal

We can use DateTimePicker from '@react-native-community/datetimepicker', instead of TextInputWithLabel.

@nikdev15
Copy link

nikdev15 commented Sep 21, 2021

Proposal
We can install a react-native npm package react-native-date-picker (https://www.npmjs.com/package/react-native-date-picker) and use the date-picker to set it as the default date-picker across both android and ios platforms as it works in both. We can easily add the date picker to any number of pages wherever it is required and customize it according to our needs. This date-picker only includes the date according to your required VBA and also works pretty same with ios and android. This date-picker package is the latest version which was updated 15 days ago.

  1. This is the date modal for android platform

date-mode-android

2) This is the date modal for ios platform

date-mode-ios

@kidroca
Copy link
Contributor

kidroca commented Sep 22, 2021

The date related components in react-native are deprecated in favour of external libraries
It seems there are no libraries that offer web support: https://reactnative.directory/?search=datepicker
I guess it will be fine to fall back to a text field (the field we currently use) for web/desktop.

I think the dev team is looking for a solution where we build a DatePicker component, even if it's just delegating everything to an external library, and then use that throughout the app. This would also allow us to fallback to text field for web/desktop (or maybe <input type="date" />)

IMO Proposals should also mention if anything else would need to change - for example the current submit handler in CompanyStep works with a string date coming from the text field - would that have to change in some way?

A nice to have would be to make the date-picker return consistent results - e.g. always return a Date instead of Date on native and string on web/desktop (even though this would work with moment).

@deetergp
Copy link
Contributor

Since the description for this issue deals specifically with iOS & Android, then going with @nikdev15's proposal seems like it would be the right choice since it makes use of the OS native date/time pickers. But @kidroca is right, a combined DatePicker component that returns the same return type across all platforms would be ideal, and a Date object would give the greatest amount of flexibility.

@nikdev15
Copy link

@deetrgrp I have understood that you need a component that returns the same type across all platforms. The way to do it is we can implement the OS native date picker and store it as a value in a variable. As @kidroca mentioned that the button component works with a string, we can override the variable to JSON format and thenuse .Stringify() methodto convert it into a string. All of this will be performed by a custom global function which will allow us to use it in all the pages where we have the date picker.

@kidroca
Copy link
Contributor

kidroca commented Sep 24, 2021

I don't see eligible proposal here - they just suggest to use an external library - we have to use an lib anyway

Even though the ticket is iOS/Android specific we still need to cover web and desktop - we can't just replace TextInputWithLabel with react-native-datepicker

@kidroca
Copy link
Contributor

kidroca commented Sep 24, 2021

Question regarding locale:
Should we use the prefered locale from Expensify.App or use the locale the user selected for the OS.
I guess usually the locale of the system and the app would be the same.

Some datepicker libraries support overriding and passing a locale, others don't

Personally I'm used to and prefer to handle dates in English even when I use a localized app (or webpage) so I would like to use the Datepicker the way the system is setup

@kidroca
Copy link
Contributor

kidroca commented Sep 24, 2021

Seems integrating this change is slightly more challenging that average

  • handling all platforms
  • the forms using a datepicker are not easily reachable -> e.g. setup bank account or setup company account.
    • Maybe I've missed something, but how are external contributors supposed to get to these pages

I've decided to submit a PR about this myself. I'll log the time under my hourly for the week
I think it would require some style decisions not mentioned on the ticket

@nikdev15
Copy link

@kidroca to answer your question earlier react-native date-picker has now introduced a new feature in their latest update of 2.0.0-alpha.23 version where the locale of the OS can be used. To give you a clear clarity

import DatePicker, { registerLocale } from "react-datepicker";
import el from "date-fns/locale/el"; // the locale you want
registerLocale("el", el); // register it with the name you want

This is the code which will get you the locale of OS

@kidroca
Copy link
Contributor

kidroca commented Sep 24, 2021

For style consideration:

I assume we'll present the picker in a popover when the text field is clicked - the text field is read only it shows the current value and serves to open the picker when it's tapped.
There's the option to use an inline picker, but it's iOS only and makes the field quite different from the rest

A picker popup should at least have a "Done" button - something to press when we're done using the spinner and are happy with the date

Do we need to include the field label in the popup. I guess it would be nice to be reminded that you're editing the incorporation date, but it might be too much.
The picker field component UI is similar and does not show a field label when the picker is popped so we should probably stick with the same

An iOS sample we can use to discuss style changes

Simulator.Screen.Recording.-.iPhone.12.-.2021-09-24.at.19.16.44.mp4

@kidroca kidroca mentioned this issue Sep 24, 2021
5 tasks
@nikdev15
Copy link

@deetergp can previous contributors simply solve the issue as I have seen @kidroca has implemented importing a date-picker although it was against his wish to do so when first-time contributors had the idea. It's absolutely barbaric to ask to post proposal ideas on GitHub as stealing of ideas can happen. When new contributors are not allowed to solve the problems why post them in the first place?

@kidroca
Copy link
Contributor

kidroca commented Sep 25, 2021

I'm sorry @nikdev15. Indeed I didn't plan on working on the ticket, but no one was hired and this is an issue with a higher priority - it doesn't have the n6-hold label, like most of the current tasks.
I'm not necessarily hired to do the work either - my PR might not be approved - it's on me
Due to past experience working on this project and the time sensitivity of the issue I've decided to submit PR and bring some items up for discussion
Otherwise I would have submit a proposal like anyone else


You proposed to import an external library and replace usages

...

Proposal
We can install a react-native npm package react-native-date-picker (https://www.npmjs.com/package/react-native-date-picker) and use the date-picker to set it as the default date-picker across both android and ios platforms as it works in both. We can easily add the date picker to any number of pages wherever it is required and customize it according to our needs. This date-picker only includes the date according to your required VBA and also works pretty same with ios and android. This date-picker package is the latest version which was updated 15 days ago.

  1. This is the date modal for android platform
date-mode-android
  1. This is the date modal for ios platform
date-mode-ios

But it's not as simple as that - we are migrating existing usages to a datepicker.
The proposal should cover how existing usages would be updated as the proposed datepicker works only on ios/Android - how would we update existing usages so that they still work on desktop and web?
How would we integrate it in the form - would it appear inline, would we replace the existing field with a button that brings the datepicker or would we make the field read only and clickable. Or at least the proposal should bring these questions up.

You can take a look to some of the closed external issues for an example on how a good proposal looks like https://github.com/Expensify/App/issues?q=is%3Aclosed+label%3AExternal

@deetergp deetergp added the Reviewing Has a PR in review label Sep 30, 2021
@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 4, 2021
@botify
Copy link

botify commented Oct 4, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.4-0 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 2021-10-11. 🎊

@botify botify changed the title Use default OS date picker for date fields [HOLD for payment 2021-10-11] Use default OS date picker for date fields Oct 4, 2021
@trjExpensify
Copy link
Contributor

@kidroca did you apply to this job on Upwork? I can't see a contract to settle it and close out.

@kidroca
Copy link
Contributor

kidroca commented Oct 14, 2021

Hey, @trjExpensify I didn't apply for this job. After the PR was accepted and merged I ended up logging the time (8h) under my hourly contract. There's no other contract to be settled.

@trjExpensify
Copy link
Contributor

Perfect, okay. Closing this out then. 👍

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 Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants