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

feat: add threaded drafts & posts #2715

Merged
merged 57 commits into from Apr 8, 2024

Conversation

maybeanerd
Copy link
Contributor

@maybeanerd maybeanerd commented Mar 29, 2024

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

  • Add a "start thread" button next to the publish button. If activated, It will show the thread index of the post, as well as the total number of posts in the thread
  • automatically prefix threaded posts with a 🧵 x/n (maybe customizable? I would start with a fixed behavior)
  • when adding a new post to a thread using the button, essentially another editor pops up below the existing one.
    • The existing ones publish button is removed.
    • The existing post can be removed from the thread again
  • since each post in a thread has their own editor, they can each include media, polls, different language and different visibility settings
    • One could think about e.g. enforcing some of these to be the same across a thread, but IMO the freedom to do whatever is quite nice
  • allow saving the entire thread as a single draft

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 make all 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.

Copy link

stackblitz bot commented Mar 29, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

netlify bot commented Mar 29, 2024

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit 7eda976
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/6613b8f468e43900080914d5

Copy link

netlify bot commented Mar 29, 2024

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit 7eda976
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/6613b8f43b9c9c00080fbeaa
😎 Deploy Preview https://deploy-preview-2715--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor Author

@maybeanerd maybeanerd left a 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

composables/masto/publish.ts Outdated Show resolved Hide resolved
composables/thread.ts Outdated Show resolved Hide resolved
composables/thread.ts Outdated Show resolved Hide resolved
@maybeanerd maybeanerd changed the title WIP: feat: add threaded drafts & posts feat: add threaded drafts & posts Mar 30, 2024
@maybeanerd
Copy link
Contributor Author

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

Copy link
Member

@shuuji3 shuuji3 left a 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)

components/publish/PublishWidget.vue Outdated Show resolved Hide resolved
composables/thread.ts Outdated Show resolved Hide resolved
maybeanerd and others added 4 commits April 7, 2024 13:28
- fix styling in publishWidget
- add TODO to not forget hardcoded workaround

Co-authored-by: TAKAHASHI Shuuji <shuuji3@gmail.com>
@maybeanerd
Copy link
Contributor Author

maybeanerd commented Apr 7, 2024

whoops i forgot to check the two thing you mentioned outside of the review comments.
->
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)

those are not yet fixed. will do soonTM

before. 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
@maybeanerd
Copy link
Contributor Author

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.

@shuuji3
Copy link
Member

shuuji3 commented Apr 7, 2024

That makes sense. Thank you for the additional change!

Copy link
Member

@shuuji3 shuuji3 left a 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. 😃

@patak-dev
Copy link
Member

This is great! Thanks a lot for this feature 🙌🏼

image

Before we merge:

  • I think we should have a tooltip in the Start thread button (like the one other buttons have next to it)
  • We should remove the "🧵 x/N" from each message. The thread emoji is equivalent to low quality posts in a lot of circles in social. Grifters that abused it to share the exact same "The best 15 CSS resources! Links in threads!!!1!1!". Let's keep it clean. Each user can decide to add a marker. I prefer threads without any x/N for example, as Elk already properly order posts. And the messages look a lot better once they are boosted deep in the thread.

@maybeanerd
Copy link
Contributor Author

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

@patak-dev
Copy link
Member

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 👍🏼

@maybeanerd
Copy link
Contributor Author

Done!

@patak-dev
Copy link
Member

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.

@patak-dev patak-dev added this pull request to the merge queue Apr 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 8, 2024
@patak-dev patak-dev added this pull request to the merge queue Apr 8, 2024
Merged via the queue into elk-zone:main with commit 1234fb2 Apr 8, 2024
13 checks passed
@maybeanerd maybeanerd deleted the feat/add-threaded-posts branch April 8, 2024 11:06
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.

None yet

7 participants