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.

Sign up for GitHub

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

Jump to bottom

[#59037] Primerize "Log time" dialog #17268

Merged
merged 90 commits into from
Jan 16, 2025
Merged

[#59037] Primerize "Log time" dialog #17268

merged 90 commits into from
Jan 16, 2025

Conversation

klaustopher
Copy link
Contributor

@klaustopher klaustopher commented Nov 25, 2024 •
edited
Loading

Ticket

https://community.openproject.org/work_packages/59037

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

ToDos

  • User input field generates the wrong input name for the hidden input. Should be time_entry[custom_field_values][14][] is timeentry[custom_field_values][14][]
  • User select adds an empty component wrapper for the hidden input field, this shows up in the UI as a seperator

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...)

@klaustopher klaustopher force-pushed the implementation/59037-primerize-log-time-dialog branch 9 times, most recently from 1b80f81 to c1fe43b Compare December 3, 2024 08:58
@klaustopher klaustopher force-pushed the implementation/59037-primerize-log-time-dialog branch 6 times, most recently from 99ba88e to 5365457 Compare December 10, 2024 10:17
@klaustopher klaustopher force-pushed the implementation/59037-primerize-log-time-dialog branch 9 times, most recently from 68695be to 834bd28 Compare December 17, 2024 08:12
@klaustopher klaustopher force-pushed the implementation/59037-primerize-log-time-dialog branch 4 times, most recently from dbb970c to 79a82db Compare December 23, 2024 09:49
@klaustopher klaustopher force-pushed the implementation/59037-primerize-log-time-dialog branch from a55ecc3 to fbdbecf Compare January 15, 2025 09:58
@klaustopher klaustopher marked this pull request as ready for review January 15, 2025 15:42
@oliverguenther oliverguenther self-requested a review January 16, 2025 06:49
oliverguenther
oliverguenther reviewed Jan 16, 2025
View reviewed changes
frontend/src/app/features/calendar/te-calendar/te-calendar.component.ts Outdated

return collection;
});
this.apiV3Service.time_entries.list({
Copy link
Member

Choose a reason for hiding this comment

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

Same here, these blocks are all reformatted without change. That makes it harder to reason about the changes. Does your editor have a "reformat only changed lines" option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's reformatting the entire file. But I am wondering why eslint is not complaining about this as being wrong after the autofix? It seems we have ambiguous settings here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undid those changes manually ... But we should really dumb down our linting config ... We have editorconfig, we have eslint, we have esprint somwhere in there as well ... I think this is very optimized for one specific editor and we should maybe only have one source of truth here.

@oliverguenther
Copy link
Member

oliverguenther commented Jan 16, 2025 •
edited
Loading

When the time zone of another user is UTC, the formatting is a bit weird IMHO

This user has a different time zone ((UTC+00:00) UTC).

If I have UTC+00:00 London selected, the offset returned is +00:00 Edinburgh. Possibly unrelated to your PR
Bildschirmfoto 2025-01-16 um 08 08 09

@oliverguenther
Copy link
Member

The datepicker for date does not open, and shows an error on page load/click

Bildschirmfoto 2025-01-16 um 08 10 15

@oliverguenther
Copy link
Member

The last case of the Hours derivation doesn't seem to work:

Hours is filled, then finish time is filled | Start time calculated as End time - hours

Start time is not set when unfocused

@oliverguenther
Copy link
Member

Bildschirmfoto 2025-01-16 um 08 14 19

Just mentioning it here. With the new autocompleter, getting the "recent" work packages tab will be lost. This will be quite annoying for a lot of users I'm sure (myself included).

@oliverguenther
Copy link
Member

Bildschirmfoto 2025-01-16 um 08 22 13

Margin between the labels for the log time dialog is wrong

oliverguenther
oliverguenther requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@oliverguenther oliverguenther left a comment

Choose a reason for hiding this comment

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

Some smaller remarks. I'd like to discuss the work package autocompleter situation in the frontend daily to get some feedback from others

@klaustopher
Copy link
Contributor Author

The last case of the Hours derivation doesn't seem to work:

Hours is filled, then finish time is filled | Start time calculated as End time - hours

Start time is not set when unfocused

done

@klaustopher
Copy link
Contributor Author

date picker is also fixed

@klaustopher
Copy link
Contributor Author

Bildschirmfoto 2025-01-16 um 08 22 13

Margin between the labels for the log time dialog is wrong

also fixed

@klaustopher klaustopher merged commit 4472737 into dev Jan 16, 2025
12 of 13 checks passed
@klaustopher klaustopher deleted the implementation/59037-primerize-log-time-dialog branch January 16, 2025 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverguenther oliverguenther oliverguenther approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@klaustopher @oliverguenther @lindenthal