-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[50954] Record progress in multiple units #15450
[50954] Record progress in multiple units #15450
Conversation
See: https://gitlab.com/gitlab-org/ruby/gems/gitlab-chronic-duration Really nice gem to handle parsing and outputting durations in multiple units.
431f846
to
a5065a4
Compare
a5065a4
to
f844feb
Compare
eslint is complaining about the |
Missing dependency in `package-lock.json`
2965574
to
58498c9
Compare
0d5a14d
to
dfb5d30
Compare
This file is sourced from Gitlab's Rails application and provided as `.js`. We therefore don't want to enforce eslint rules on it to keep it prisitine with its upstream source.
dfb5d30
to
36b3764
Compare
36b3764
to
147d6ea
Compare
8885105
to
0be7415
Compare
682e928
to
da19e6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I played a little with it. It responds really well. Good job!
3 things I wonder about, and probably @psatyal can help us on it:
- what do we do with text longer than the allowed size? Currently it's clipping. Do we extend the default width?
- what about the precision? When entering 1m, it can't represent it accurately with only 2 decimals. It results to "1m 12s". Not a big deal, but it's kinda surprising to enter "2h 12m" and end up with "2h 12m 0.0000000001s", or enter "2h 13m" and end up with "2h 13m 12.00000000001s". Should we switch from floating points holding nb of hours to integer holding nb of seconds?
- when omitting the "m", or before entering it, the value is interpreted as hours. It would be better if interpreted as minutes. This is especially visible when entering a value for the first time: with w -> rw, and typing each character one by one, it gives this:
- work -> remaining work (when 100%)
- 2 -> 2h
- 2h -> 2h
- 2h1 -> 3h (would have been expecting 2h 1m)
- 2h15 -> 17h (would have been expecting 2h 15m)
- 2h15m -> 2h 15m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve nor request changes on this PR because I created it 😅
The multiple units display is really nice, and the way it's used in the popover is smooth and does not get in the way. That's nice!
I have some tiny remarks here and there.
I also have the feeling that the precision is annoying us. For instance:
- entering "2h 13m" leads to 7980 seconds, converted to 2.216666667 hours, rounded to 2.22 (2 decimals). This value 2.22 is the one being saved in database.
- When frontend is rendering it, it has 2 ways of rendering it:
- the modal is rendering it through
DurationConverter.output
, which for 2.22 will render "2h 13m 12s" - the angular part is using a different route: the 2.22 is converted in the representer using
datetime_formatter.format_duration_from_hours
, so 2.22 is converted to iso duration "PT2H13M12S". This duration is then handled in the timezone service which converts it to hours withNumber(moment.duration(durationString).asHours().toFixed(2))
which is 2.22, converted to seconds which gives 7992.000000000001, and then it is converted to chronic duration which gives "2h 13m 12.000000000001s"
- the modal is rendering it through
So the rounding is always false, and sometimes more false than others. I think the second one can be fixed by avoiding converting iso duration to hours and then to second: by converting iso duration directly to seconds, we could avoid precision loss.
Another thing we could do is to avoid displaying seconds entirely by making the hours to seconds conversion remove them. Something like (hours * 3600 + 15).to_i / 60 * 60
. For 2.22 hours, it would give 7980 which converts to "2h 13m". No seconds, no errors.
WDYT?
app/controllers/admin/settings/working_days_and_hours_settings_controller.rb
Outdated
Show resolved
Hide resolved
app/contracts/settings/working_days_and_hours_params_contract.rb
Outdated
Show resolved
Hide resolved
app/views/admin/settings/working_days_and_hours_settings/show.html.erb
Outdated
Show resolved
Hide resolved
<div class="op-toast -warning"> | ||
<div class="op-toast--content"> | ||
<p> | ||
<%= t("working_days.warning").html_safe %> | ||
</p> | ||
</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this warning should belong to the next section. Currently it looks like it belongs to the wrong one.
Maybe @psatyal can confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts @psatyal ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the commit adfa1bd
in case we agree this is where it should be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seen with Parimal during today's frontend meeting:
- the text has been slightly reworded
- the warning is moved on top of the section, so the warning can be read before doing the changes
He also asked if it's possible when clicking "Apply changes" to display the warning message about changed days only if there are actually changed days. I'll do another review to specifically ask for this so we can track it more easily.
ddafeee
to
98b00ab
Compare
Also: - reword the warning message slightly to remove the "click apply to apply" part. - remove some html_safe calls that are not needed anymore (especially since 2f82d73).
4ed12ac
to
1111687
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An additional request from Parimal while discussing this screen during today's frontend meeting
|
||
I18n.t('date.day_names').rotate.zip(WeekDay::DAY_RANGE), | ||
direction: :horizontal %> | ||
I18n.t('date.day_names').rotate.zip(WeekDay::DAY_RANGE), | ||
direction: :horizontal %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create a bug work package for that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve because I created the PR, but I approve it 👍
https://community.openproject.org/wp/50954