- Notifications
You must be signed in to change notification settings - Fork583
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat: add threaded drafts & posts#2715
Conversation
|
✅ Deploy Preview forelk-docs canceled.
|
✅ Deploy Preview forelk-zone ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just a few notes for myself
looks like most likely this should be somehow be combined on a lower level with drafts
a138bf7 tof4a99bcCompareits still broken, but this way its broken consistently
now the reactivity of a draft array should work
remove support for a usecase that doesnt exist
So, I think visually this is something that kind of is okay right now, and the drafts include the entire thread now I'll focus on actually being able to post the thread, and adding the thread count to the posts themselves, next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@maybeanerd Sorry for letting you resolve code conflicts 🙏🏻
I made a few comments.
Also, there are two issues I noticed when composing in the home timeline:
- When publishing the normal (non-threaded) post, the text in the editor should be cleared but now the text remains there.
- After publishing the threaded posts, the page is navigated to the last thread page. But Elk usually stays home timeline page without navigation. So it may be better to keep the same behavior. (but this one can be fixed by the following PR since only affects to the new draft feature)
- fix styling in publishWidget- add TODO to not forget hardcoded workaroundCo-authored-by: TAKAHASHI Shuuji <shuuji3@gmail.com>
2820e95 toe0f14c9Comparewhoops i forgot to check the two thing you mentioned outside of the review comments. those are not yet fixed. will do soonTM |
0295acc to48c04f0Comparebefore. all threads were detected as response since their last message is always a response to the second last message
before, if a status was not part of a thread, after posting it it was falsely kept in the editor
Now they should also be fixed ✨ Regarding navigation after a post: elk does navigate, but only if the post was a reply. Since threads reply to eachother, this falsely always triggered it, but should now only trigger if the first message of the thread was a reply. |
That makes sense. Thank you for the additional change! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this new thread feature is good enough now to test in the Canary Elk. 😃
This is great! Thanks a lot for this feature 🙌🏼  Before we merge:
|
Fully agree with the tooltip, will do! Concerning the prefix, I thought we'd later add a setting and make it optional - but we can also entirely remove it, I included it mostly because it was requested in the original issue |
I don't think the prefix will have enough demand to justify a setting (that it would also involve configuring the emoji, format, etc). Let's keep it simple and remove it 👍🏼 |
Done! |
Nice! Let's merge it 🚀 One detail, I think the current remove message button is a bit confusing. You can't remove the last message in the thread for example. And the delete button being in the bottom-right makes it closer to the next message than the one that is going to be removed. I think it would be better to have a new red cross circular button in the top-right of each message, and then the last one can also be removed. |
implements#2148
This adds thread drafting and publishing support.
Threads are prefixed by default, but this is something that could definitely be added to settings and become configurable in the future.
The internals have been kept mostly as they were, reusing many existing composables. In the future, there is probably a lot of opportunity to streamline the entire state and dataflow, but I didn't want to rewrite everything as part of this PR.
Statuses that are added to a thread copy it's settings (e.g. language and visibility) but can be configured differently.
original description:
I'm currently experimenting around and trying out different things, but I wanted to be transparent about it and open up the possibility for early feedback, which is why I'm opening this Draft PR.
Please feel free to provide feedback on everything, especially the part on implementation details/architecture
Here's my current plan:
UX
🧵 x/n(maybe customizable? I would start with a fixed behavior)technical architecture
After playing around with the state that this PR has as of now, I noticed that mixing drafts and threads is a real mess.
I think what I will try doing is makeall posting scenarios be a thread under the hood, but handle threads with length == 1 a bit differently (theyre just normal posts)
This should also allow saving an entire thread, or single posts, as distinct drafts that dont conflict by accident.