search

beakerbrowser / fritter

A peer-to-peer social feed app. (proof of concept)
dat://fritter.hashbase.io
MIT License
363 stars 36 forks source link

Mentions #28

Closed SaFrMo closed 6 years ago

SaFrMo commented 6 years ago

Okay! Ready for review!

Notes:

Let me know if you run into any bugs!

taravancil commented 6 years ago

Yay! I’m so excited to take a look at this. FYI Paul and I are in San Francisco right now so we probably won’t have a chance to look at this for a few days.

On Thu, Feb 1, 2018 at 06:57 Sander Moolin notifications@github.com wrote:

Okay! Ready for review!

Notes:

  • Requires libfritter PR#2 https://github.com/beakerbrowser/libfritter/pull/2 to save mentions
  • The new post form and the thread reply form now pull from the same element, com/new-post-form. A few things to note about the revamped new-post-form:
    • You can pass an object with source, onSubmit, and/or onKeyUp when creating a new-post-form.
      • source is the property of app to reference when making changes or updates to a post draft. For example, setting source = 'replyDraftText' in com/thread makes the form change app.replyDraftText instead of the default app.postDraftText.
      • onSubmit and onKeyup override the default submit/keyup behaviors.
    • The new post form should create a post through utils.buildPost, feeding in any existing information like text, thread root, thread parent, etc. buildPost builds out the mentions in the text, saves them to the passed object, and returns that modified object.

Let me know if you run into any bugs!

You can view, comment on, or merge this pull request online at:

https://github.com/beakerbrowser/fritter/pull/28 Commit Summary

  • Starting mentions UI element in new posts
  • Adding mentions to posts (requires update to libfritter)
  • Merge remote-tracking branch 'upstream/master' into mentions
  • WIP on mentions
  • Added multiple mentions capability, search case insensitivity
  • Minor styling changes
  • Mentions dropdown styling & functionality
  • Removed unused & duplicate mentions from submitted post
  • Improved unique mention check
  • Added mentions to notification index
  • Formatting, trimming extra code
  • Added mention link parsing to feed items
  • Fixed textarea trim issue
  • Composer working as reply input
  • Fixed spacing issue after a new mention
  • Removed a couple console.logs
  • Fixed dat.json

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/beakerbrowser/fritter/pull/28, or mute the thread https://github.com/notifications/unsubscribe-auth/AHO8QXmMuCsyFfoMq89RVYsZfsdrROXgks5tQdC8gaJpZM4R1vXP .

SaFrMo commented 6 years ago

Sounds good to me! Looking forward to it!

pfrazee commented 6 years ago

I looked over the code (more than a skim but not in depth). Good job matching style and patterns. I see no obvious logic errors. @taravancil give it a test and if it works, I'm 👍 to merge!

Nice work @SaFrMo !

pfrazee commented 6 years ago

Ok I finally got a chance to try this out. Nice work!

This feature is hard to get right and you've done a great job starting it. Unfortunately taking it to the finish line is going to be a real pain. Here are some issues:

  1. The textarea lost its default state (contracted with placeholder text and no submit button). Is there any way we can get that back?
  2. The mention suggestion list really should hover near the cursor. It being in a fixed position is pretty confusing.
  3. Tabbing to the mention options is kind of confusing, it really ought to work more like github's where you can use down/up arrows.
  4. The content-editable is close to working how I'd expect but it's not 100%. Do we need the content-editable? I'd suggest we ditch it if the answer isn't 100% yes.
  5. There are some random spaces inserted around the mentions in the output text, I assume by the content-editable.

The code for this thing fell into total disrepair so I hesitate to share it, but https://www.npmjs.com/package/suggest-box is what is used in Patchwork and it has a lot of useful snippets to steal from.

SaFrMo commented 6 years ago

Great notes @pfrazee ! I'll dig in and have some more complete answers at the start of next week. Thanks! 😄

SaFrMo commented 6 years ago
  1. Definitely! I'll add that back in.
  2. Sounds good! Github treats the @ as the starting point, so I'll set up Fritter's to be similar.
  3. Agreed, I'll add that in as well!
  4. We need the content-editable if we're going to have in-fret mention labels like Twitter, but if we handle it Github-style, we'd be able to do without. I can switch back to an input field (or textarea, I forget which it was before) so we see what it looks like!
  5. Noted! Should be fixed by the change to 4.

I'll get back on this and tag you when the changes are ready!

SaFrMo commented 6 years ago

FYI, I'm starting a new branch on my fork for the updates, since most of the changes in this PR have to do with the contenteditable setup. It's called mentions-v2 - I'll close this PR and submit a new one for that branch when it's ready!

To-do list:

pfrazee commented 6 years ago

Ok sounds good!

On Tue, Feb 20, 2018 at 8:02 AM, Sander Moolin notifications@github.com wrote:

FYI, I'm starting a new branch on my fork for the updates, since most of the changes in this PR have to do with the contenteditable setup. It's called mentions-v2 https://github.com/SaFrMo/fritter/tree/mentions-v2 - I'll close this PR and submit a new one for that branch when it's ready!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/beakerbrowser/fritter/pull/28#issuecomment-366986173, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNhU6N8jKScc7Tbw8HdA9AGrtp43eEeks5tWtBhgaJpZM4R1vXP .

SaFrMo commented 6 years ago

Couple more bugs to fix, then this should be ready by the end of the week (or start of next week at the latest)!

Quick question for @taravancil and @pfrazee - there's some really similar code here in the new post form and here in the reply form. I can see some style differences and the ability to save both a new post draft and a reply draft, but I'm wondering if it'd be better to consolidate both forms to a single file? I'd argue that the ability to save the two different kinds of drafts isn't as handy to the user as the decreased repetition would be for devs, and the styling could be accomplished by some flex ordering or a conditional.

Not super critical (and not waiting on any answer for to be able to wrap up), just wanted to get your thoughts!

pfrazee commented 6 years ago

Yeah if you want to consolidate those forms, and you think it would work better, feel free to merge them. If not that's cool too.

SaFrMo commented 6 years ago

Closing in favor of #30