[FIX] hr_holidays: dynamic accrual days #4904
Conversation
|
This PR targets the un-managed branch odoo-dev/odoo:master-hr-onboarding-haabo, it needs to be retargeted before it can be merged. |
hazemayman1
force-pushed
the
master-hr-onboarding-ux_accrual-haabo
branch
2 times, most recently
from
3207f98 to
d7d62df
Compare
Mahmoudk3m
left a comment
Mahmoudk3m
left a comment
There was a problem hiding this comment.
Thanks for your work 🚀
Just a few comments.
| let date = new Date(2024, selectedMonth, 0); | ||
| let days = date.getDate(); |
There was a problem hiding this comment.
| let date = new Date(2024, selectedMonth, 0); | |
| let days = date.getDate(); | |
| const date = new Date(2024, selectedMonth, 0); | |
| const days = date.getDate(); |
Use const instead of let where values don't change:
| const selectedMonth = this.props.record.data[this.props.selectedMonth]; | ||
| let date = new Date(2024, selectedMonth, 0); | ||
| let days = date.getDate(); | ||
| let newChoicesList = Array.from({length: days}, (_, i) => [(i + 1).toString(),(i + 1).toString()]) |
There was a problem hiding this comment.
| let newChoicesList = Array.from({length: days}, (_, i) => [(i + 1).toString(),(i + 1).toString()]) | |
| let newChoicesList = Array.from({length: days}, (_, i) => [(i + 1).toString(), (i + 1).toString()]) |
missing space after comma
|
|
||
| get options() { | ||
| const selectedMonth = this.props.record.data[this.props.selectedMonth]; | ||
| let date = new Date(2024, selectedMonth, 0); |
There was a problem hiding this comment.
I would add a comment to explain why I used 2024 specifically, because it might be a bit confusing.
|
|
||
|
|
||
| get options() { | ||
| const selectedMonth = this.props.record.data[this.props.selectedMonth]; |
There was a problem hiding this comment.
If selectedMonth is undefined or invalid, this could fail silently.
There was a problem hiding this comment.
added a condition to prompt the user and not give him any day options to select from
There was a problem hiding this comment.
If a user has selected "31" for the day and then changes the month to one with only 30 days, what happens? 🤔
There was a problem hiding this comment.
The widget automatically adjusts I think it's because it's watching on the selectedMonth prop to recall the function on change
arpi-odoo
force-pushed
the
master-hr-onboarding-haabo
branch
from
8724928 to
fdc2c51
Compare
hazemayman1
force-pushed
the
master-hr-onboarding-ux_accrual-haabo
branch
from
d7d62df to
ac4b195
Compare
| if (isNaN(selectedMonth) || selectedMonth < 1 || selectedMonth > 12){ | ||
| alert("Please select a valid month."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Using alert is not really standard for Odoo. You could raise an error if you want to do that.
What I had meant was to just check if it exists before the condition
Changed the {day} dropdown to adapt to the selected month. It was previously set to 31 for all months which allowed the user to select the 31st on a 28 or 30 day month for example.
task-5392293
hazemayman1
force-pushed
the
master-hr-onboarding-ux_accrual-haabo
branch
from
ac4b195 to
fd32130
Compare
Changed the {day} dropdown to adapt to the selected month. It was previously set to 31 for all months which allowed the user to select the 31st on a 28 or 30 day month for example.
task-5392293