Ellpeck / ObsidianSimpleTimeTracker

Multi-purpose time trackers for your notes!
MIT License
132 stars 19 forks source link

Editable timestamps and mobile horizontal scroll #21

Closed bagaag closed 12 months ago

bagaag commented 1 year ago

This PR resolves two open issues. I've tested these on both the Linux and Android clients.

Ability to edit timestamps of entries

Fixes #9

I extended the logic used for the editable entry label to add the ability to edit time stamps as well. Doing so resulted in a lot of repeated code, so I consolidated management of editable fields into a class.

Editable time stamps are formatted using the user's setting, which requires that format be specific enough to convert back into a timestamp. This could cause confusion if a user sets a format that does not convert back into a timestamp. The alternative would be to use a fixed format or a separate setting for editing timestamps.

Entry buttons cut off in mobile

Fixes #8

I encountered this bug while testing the change on mobile. This is a simple fix by making the container horizontally scrollable if needed.

bagaag commented 1 year ago

The width = 10 was not meant to be left there. I've removed it.

The input style was needed to achieve consistent formatting on desktop and mobile (Android). Without that style, the boxes were way too wide on both platforms and became too narrow to edit on Android.

As for the format setting, I actually just use "HH:mm:ss" because my entries never span more than 1 day, so I only care about relative time. It works for that purpose. The default setting of "YY-MM-DD hh:mm:ss" is actually problematic because "hh" is 12 hrs, not 24. We'd need to add an "a" (AM/PM) to that format for it to work properly. Let me know if you prefer a hard-coded format of "YY-MM-DD HH:mm:ss" or "YY-MM-DD hh:mm:ss a" and I can make that change. Personally, I like "HH:mm:ss" as it's smaller and works if entries don't span dates. But letting users set this could definitely cause confusion.

On Mon, Jul 3, 2023 at 12:54 PM Ell @.***> wrote:

@.**** commented on this pull request.

In src/tracker.ts https://github.com/Ellpeck/ObsidianSimpleTimeTracker/pull/21#discussion_r1251113084 :

@@ -268,18 +273,48 @@ function createTableSection(entry: Entry, settings: SimpleTimeTrackerSettings): return ret; }

+class EditableField {

  • cell: HTMLTableCellElement;
  • label: HTMLSpanElement;
  • box: TextComponent;
  • constructor(row: HTMLTableRowElement, indent: number, value: string) {
  • this.cell = row.createEl("td");
  • this.label = this.cell.createEl("span", { text: value });
  • this.label.style.marginLeft = ${indent}em;
  • this.box = new TextComponent(this.cell).setValue(value);
  • this.box.inputEl.width = 10;

Why are you setting the width to 10 here?

In styles.css https://github.com/Ellpeck/ObsidianSimpleTimeTracker/pull/21#discussion_r1251113501 :

+.simple-time-tracker-input {

  • max-width: 150px;
  • min-width: 100px;

What are these new style settings for?

— Reply to this email directly, view it on GitHub https://github.com/Ellpeck/ObsidianSimpleTimeTracker/pull/21#pullrequestreview-1511526382, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASN2SAGIAWHTOVSM722Q2TXOL2MZANCNFSM6AAAAAAZ4WBR4U . You are receiving this because you authored the thread.Message ID: @.***>

Ellpeck commented 1 year ago

Thanks for the explanations!!

I think hardcoding a 24 hour format with four year digits would make most sense, so YYYY-MM-DD HH:mm:ss. I hadn't actually noticed the HH/hh discrepancy in the original code before, so that may also be something that needs fixing at some point!

bagaag commented 1 year ago

Sounds good. I should be able to make that change in the next couple of days.

If you like, I can make the change of hh to HH in settings.ts while I'm at it.

Ellpeck commented 1 year ago

If you like, I can make the change of hh to HH in settings.ts while I'm at it.

Yes, that would be lovely! Thanks.

bagaag commented 1 year ago

Editable timestamps now use a fixed format, which is specified as a default in settings in case you decide to expose that in the future. In the meantime, power users can customize it in their data.json file.

Ellpeck commented 1 year ago

Looks great, thanks so much again!

I'll merge this as soon as I get a chance to work on the plugin again, so that I can test it properly right away.