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.

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

fix repeater for simple timestamps #568

Merged
merged 11 commits into from
Nov 21, 2020

Conversation

tarnung
Copy link
Collaborator

@tarnung tarnung commented Nov 13, 2020

Repeaters on simple timestamps are broken.
When the todo state changes, the last repeat property is created, a log entry is written and the planning item in the redux state is updated. The part in the title or description that represents the timestamp is not touched though, so while the item moves in the agenda, the change is not represented in text and is not persisted.

This fix connects the timestamps in title and description with their representation in planning items by keeping the id instead of generating a new one. It fixes the issue. I'm quite sure but not 100% certain that having planning items with the same id as parts of title or description won't cause problems. It makes sense, since the items with the same id represent the same thing. If it's preferred I could instead put a timestampId on the planning items and let them have their own unique id.

@munen munen force-pushed the fix/repeaters-on-timestamps branch from f204a41 to 5bb3ad2 Compare November 15, 2020 15:27
@munen
Copy link
Collaborator

munen commented Nov 15, 2020

I've added a reducer test case for it which fails on master, but works on this branch. I've also confirmed functionality manually.

What's left is to fix the eslint warning:

image

@tarnung
Copy link
Collaborator Author

tarnung commented Nov 16, 2020

I added support for deleting timestamps.
In the process I found a bug in removePlanningItem. When the timestamp editor is opened by clicking on a timestamp that does not belong to the currently selected header, removePlanningItem tries to remove the item from the wrong header.

@munen
Copy link
Collaborator

munen commented Nov 21, 2020

Great job, again 👍

I'll add it to the changelog and will merge it in!

@munen munen force-pushed the fix/repeaters-on-timestamps branch from 8de7c73 to 06b9065 Compare November 21, 2020 12:56
@munen munen merged commit 76b46ae into 200ok-ch:master Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants