Search and Task List in one Modal #751
Conversation
|
In #217 there was a discussion about the "set deadline" and "set scheduled" buttons immediately creating timestamps. An argument in favour was that you otherwise would need more clicks to create the timestamp. I don't fully share that concern as you probably don't want to set a scheduled event or a deadline to "now" so you will need several more clicks to configure the timestamp anyways. In addition if I use the new in modal buttons to switch between editing different parts of the header i have another chance to accidentally create timestamps. If I delete the timestamp I get thrown out of the modal - which none of the other header focussed modals that you can switch between does on deletion of say a tag or a property. If there are strong opinions against that, it's easy to change it back since it's one commit. Quick aside:
|
|
I don't plan on any more direct additions to this PR. It seems like a nice package as is. The only thing I found is that there now only is a #search in the url when either search or task list are opened. |
|
@tarnung Thank you for the update! 🙏 I'm currently testing on iOS and on the Desktop. I've updated the last test in #748. The final issue before we can merge #748 is the iOS regression on editing descriptions for which I'm currently testing options. Since there are many (great!) changes in the last PRs, would you consider doing a code walk-through with me? We could do a screen-sharing session, or I could offer office space in Glarus where we could also have a coffee or tea together. Of course, all of these are optional. The alternative is that I'll do my best to do a code review at my speed and time and merge when I've gotten through. |
|
If the changes in #748, #747 and in here are all to be merged, it might be easiest to only merge 751 since all code is already in here. If you prefer to go one by one and you can perform the necessary rebasing magic, that's fine with me too. We can plan a remote code walkthrough. Just tell me when you have time to spare. I'm in no hurry since I can already use the changes on staging anyway :) |
Ack. I see no blockers to look straight at #751 for merging. I'm only looking at #748, because it is slightly smaller and introduced the iOS description scrolling issue.
Sweet! This week is completely booked (work, templestay, meditation retreat). But next week, I'm pretty flexible from Monday to Sunday except for Monday afternoon/evening. If you have any preferences, the chances are high that we can do it, then. If you have no preferences, then I suggest Tuesday the 23rd at 10 am. |
|
Would Monday 22nd 10am work for you? That or Friday would be my best options for a spot during the day. Otherwise we'd have to make it an evening. |
That works well! I'll send you a Zoom invite via your hey email address. |
Rationale: The `<Motion>` component has listeners on `touchstart`, `touchmove`, `touchend` and `touchcancel`: https://github.com/200ok-ch/organice/blob/72b46bf80ade330f59d0747246f0c8b5547ab227/src/components/UI/Drawer/index.js#L64-L67 The `<textarea>` of `<DescriptionEditorModal>` is embedded within `<Motion>`. These touch event listeners interfere with the ability to scroll on iOS.
State: It accepts multi-line input now. There's two UX
caviats, yet:
- The modal does not close on "Add"
- When the multi-line note is saved to the document, it
becomes just one line (\n is removed)
Fixes two bugs:
- Before, adding a multi-line note that featured lines < 70
characters long, they would get joined.
- Before, even if the multi-line notes wouldn't get joined, only
the second line would be indented to be within the note. The
lines from the third line would not have indentation.
|
Screenshot showcasing the functionality of the last commit: |
|
@schoettl FYI, with merging #751 from tarnung, the user can input multi-line notes. I've made these changes to the code from you: Here's a showcase of what it looks like now: #751 (comment) I hope you're ok with these changes. Since this PR is already huge and builds on several other huge PRs, I think these changes are good for you, too, I have not opened a separate PR. |
|
I have updated Staging with the current state of #751. |
|
Seems good to me. At least from a user perspective. I can't speak to the effects of mulit line notes on the parser etc. |
|
Using a capture template looks like this on iOS:
I’ll look into it on Monday. I only just realized it. |
tarnung
force-pushed
the
feat/search-tasklist-tabs
branch
from
68e152e
to
02229b7
Compare
This PR sits on top of #748 and #747 and adresses task one in #211.
Feel free to discuss if this is a sensible behaviour.
A caveats:
This is a design refactor. It still uses SearchModal and TaskListModal under the hood so there was no cleaning up of duplicated functionality.