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

Jalaali (Persian) Date input support #1205

Open
wants to merge 4 commits into
base: v1_develop
Choose a base branch
from

Conversation

inamvar
Copy link

@inamvar inamvar commented Apr 12, 2021

resolving issue #1204

@tznind
Copy link
Collaborator

tznind commented Apr 27, 2021

Is this something that could be done with inheritance? By making GetShortFormat and maybe the Date property virtual (perhaps combined with making shortFormat and longFormat public (or readonly/protected)?

Then users could create their own custom date formats and calendars types? Could even have a Jalaali version of DateField (either in core or Scenarios) to show how to implement.

@inamvar
Copy link
Author

inamvar commented Apr 27, 2021

Is this something that could be done with inheritance? By making GetShortFormat and maybe the Date property virtual (perhaps combined with making shortFormat and longFormat public (or readonly/protected)?

Then users could create their own custom date formats and calendars types? Could even have a Jalaali version of DateField (either in core or Scenarios) to show how to implement.

Sounds good! It needs to change some methods identifiers in DateField class and set them as virtual methods to be able to override them in the inherited class.

@tig
Copy link
Collaborator

tig commented Apr 27, 2021

Is this something that could be done with inheritance? By making GetShortFormat and maybe the Date property virtual (perhaps combined with making shortFormat and longFormat public (or readonly/protected)?
Then users could create their own custom date formats and calendars types? Could even have a Jalaali version of DateField (either in core or Scenarios) to show how to implement.

Sounds good! It needs to change some methods identifiers in DateField class and set them as virtual methods to be able to override them in the inherited class.

this seems like a good approach.

Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@inamvar,

We currently have zero unit tests for DateField. Via other tests, it has about 53% code coverage.

I'd like to ask you to add a DateFieldTest.cs module with some tests that would get this closer to 80%. Would you please do this as part of this PR?

Examples:

  • the constructors
  • functions like GetLongFormat, 'GetDate', etc...
  • anywhere you see any sort of logic that maniuplates stuff

Thanks!!!!

@@ -47,9 +48,11 @@ public class DateField : TextField {
/// <param name="y">The y coordinate.</param>
/// <param name="date">Initial date contents.</param>
/// <param name="isShort">If true, shows only two digits for the year.</param>
public DateField (int x, int y, DateTime date, bool isShort = false) : base (x, y, isShort ? 10 : 12, "")
/// <param name="isJalaali">If true, parse will convert jalaali input fo georgian date</param>
public DateField (int x, int y, DateTime date, bool isShort = false, bool isJalaali = false) : base (x, y, isShort ? 10 : 12, "")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few unit tests that prove the various constructors work as they should would be good.

@@ -377,6 +410,28 @@ public virtual void OnDateChanged (DateTimeEventArgs<DateTime> args)
{
DateChanged?.Invoke (args);
}

string ToJalaaliString (string format, DateTime date)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have a unit test for this.

@tig
Copy link
Collaborator

tig commented May 30, 2021

Hi @inamvar - We'd love to integrate this PR. Will you have a chance soon to address the changes requested?

@tig
Copy link
Collaborator

tig commented Jul 21, 2021

@inamvar - are you still interested in helping with this?

@tig
Copy link
Collaborator

tig commented Oct 20, 2022

@inamvar - I'm marking this as a Draft PR. Hope you come back some time and help us get this merged in!

@tig tig marked this pull request as draft October 20, 2022 14:29
@inamvar inamvar marked this pull request as ready for review October 23, 2022 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants