fix: 6842 added property firstDayOfWeek #9497
Conversation
vibhandikyash
added
the
App Viewers Pod
|
/ok-to-test sha=777c050 |
|
Unable to find test scripts. Please add necessary tests to the PR. |
4 similar comments
|
Unable to find test scripts. Please add necessary tests to the PR. |
|
Unable to find test scripts. Please add necessary tests to the PR. |
|
Unable to find test scripts. Please add necessary tests to the PR. |
|
Unable to find test scripts. Please add necessary tests to the PR. |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/1525315814. |
|
/ok-to-test sha=a02e5e6 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/1544132287. |
vibhandikyash
requested review from
SatishGandham
and removed request for
vicky-primathon
| @@ -156,6 +156,9 @@ class DatePickerComponent extends React.Component< | |||
| <DateInput | |||
| className={this.props.isLoading ? "bp3-skeleton" : ""} | |||
| closeOnSelection={this.props.closeOnSelection} | |||
| dayPickerProps={{ | |||
There was a problem hiding this comment.
Are we expecting more props here, why not firstDayOfWeek instead of dayPickerProps?
There was a problem hiding this comment.
No, we are not expecting more props support right now, firstDayOfWeek is not supported directly.
As per blueprint, this is supported into dayPickerProps. @SatishGandham
|
/ok-to-test sha=2abacb7 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/1577043545. |
|
@vibhandikyash Few observations while testing the Deploy Preview
|
|
/ok-to-test sha=fd3934b |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/1586929442. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/get-appsmith/appsmith/HsCP2cFUQk6gARxFXgqpWarvFvLk |
|
/ok-to-test sha=750dfc1 |
|
@vibhandikyash Tested this PR and please find the status below, Issue 1 - Fixed New Enhancement Issue 5 - Enhancement - Instead of values 0-6 we can pass values in String (Sunday, Monday, Tuesday, etc) which will be more helpful |
|
@YogeshJayaseelan @somangshu plz let us know can we do this? |
|
@techbhavin Ill agree with you, Given that this is set by the developer we can consider that they are aware of the concept, I dont see much of a value add. However, we need to clearly show this in the tooltip helper of the property to denote the index, ex 0 for sunday and so on and so forth. Regardless of the of the critically, To implement this we could create a mapping of the index and String. (I still feel this is an overdo, even from the user point of view) |
|
@somangshu can we add a drop-down instead of Textbox for select First day of week ? |
|
@techbhavin What if somebody wants to use an expression to set this dynamically. I suggest not spending much time to improve this. Rather focus on validation, helper text, test coverage for the fix. We can have the dropdown, Given that its not going to take much time to implement, test and resolve more issues. Maybe create another enhancement for the future! |
|
/ok-to-test sha=b3cc194 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/1629951129. |
|
/ok-to-test sha=f847639 |
|
/ok-to-test sha=f847639 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/1632748727. |
Description
Fixes #6842
Type of change
How Has This Been Tested?
Checklist:
Test coverage results 🧪
🔴 Total coverage has decreased