Closed (fixed)
Project:
Log entity
Version:
2.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
13 Oct 2020 at 17:03 UTC
Updated:
29 Oct 2020 at 12:19 UTC
Jump to comment: Most recent, Most recent file
There are a few improvements that Log could benefit from:
- Move "authored on" and "authored by" into vertical tabs (similar to Nodes)
- Add "C" buttons to timestamp field (like "authored on")
- Allow submitting without hours (set to midnight)
- Make timestamp and state fields not required in form (timestamp will be autofilled)
- Should not be possible to end up with a blank log name: Log form should make name required if name_pattern is empty
- Should we change /admin/structure/log_type to /admin/structure/log-type?
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3176676-2.patch | 6.44 KB | pcambra |
Comments
Comment #2
pcambraSome of these are quite simple and some are not, here's a patch that includes:
- Move "authored on" and "authored by" into vertical tabs (similar to Nodes)
Included in the patch. However, it is not moved to the sidebar region because that's on the theme layer (twig files actually)
- Add "C" buttons to timestamp field (like "authored on")
Included in the patch.
- Allow submitting without hours (set to midnight)
This needs discussion, as the widget that resets the date/time is HTML5 default so it is on the browser side. I think there's actually a core bug behind this because fields such as the created state that "Leave blank to use the time of form submission.", but if you leave it empty, it fails validation "The Authored on date is invalid. Please enter a date in the format 2020-10-13 17:09:19.", will try to add a test and open a core issue for it. We should open a follow up for this one.
- Make timestamp and state fields not required in form (timestamp will be autofilled)
Included in the patch.
- Should not be possible to end up with a blank log name: Log form should make name required if name_pattern is empty
This definitely makes sense but I've decided not to include changes on this area in the patch. I think we need to take it back to the whiteboard a little bit in a followup.
The required/not required HTML5 validation has this interesting issue opened #1797438: HTML5 validation is preventing form submit and not fully accessible that definitely impact us (in terms of showing programmatically whether a required field might not be such) but our main issue is that, because the log type name pattern might include data that is calculated after the save, we use the postSave method in the entity storage.
However, if we use postSave, we don't have title when we save, so if it is required, it will fail. However, the right way to ensure that a log never ever ends without title, is to set it required.
I'm not entirely sure what's the good solution for this tbh.
- Should we change /admin/structure/log_type to /admin/structure/log-type?
Included in the patch.
Comment #3
m.stentaGreat! We can tackle that in #3151246: [META] farmOS 2.x UI/UX.
I knew this one would be tricky. ;-)
I agree - this can be split off to a separate issue, and it's a minor "nice to have" IMO. I have some thoughts for how to do this via simple JavaScript. I may have documented these ideas somewhere already... (thinking face emoji) We may even just consider adding this in farmOS only.
Created a separate issue: #3176831: It is possible to save a log without a name
Comment #5
m.stentaMerged all the changes from this patch - thanks @pcambra - all works great!
Comment #6
m.stentaOops - I just realized that making the "Status" field optional means that it's possible to save a log without a status. And that causes other things to break because of assumptions that a status is always set (eg: editing the log results in
Error: Call to a member function getLabel() on null).I honestly forget why I said:
It doesn't make sense in hindsight.
I am going to revert just that piece of this.
(I think it may also be the reason my tests in #3176835: Regenerate log names on update using the name pattern are failing.)
Comment #8
m.stenta