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

Add optional text input for link posts #558

Merged
merged 6 commits into from
Oct 22, 2023
Merged

Conversation

rleed
Copy link
Contributor

@rleed rleed commented Oct 12, 2023

To resolve #504.

image

Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at these changes, I realized that it should be a lot easier to just also set text and not only url of the item instead of creating a comment as a separate item. This has the following advantages:

  • can reuse existing create_item postgres function, so no db migration with support for noCosts needed
  • fee escalation can work as before: no need to mention something like free, but compounds any future spam penalty which not every user might understand
  • it just works™ without any backend changes 🎉 (did not test if it really requires zero backend changes but i have good faith belief in that)
  • consistent edit UX: you can edit everything you were able to create in the same form

Another thing is that it doesn't "smell good" to add noCosts as an exception to create_item (which was meant to be a generic function) for a single use case.

It also might be better UX if the provided summary actually shows up at the top for every user instead of as a separate comment which might get lost if there are many comments? Using item.text kind of works like a pinned comment in that case (except it's just regular UX like for discussion posts)

Thanks for putting in the work! It wasn't clear to me before that creating a separate comment might lead to all of this.

components/adv-post-form.js Outdated Show resolved Hide resolved
components/adv-post-form.js Outdated Show resolved Hide resolved
@ekzyis
Copy link
Member

ekzyis commented Oct 17, 2023

Thinking more about this, I wondered if it would break some assumptions in our code if link posts now suddenly also can have text set to something - like code where we want to distinguish between link posts and discussion posts.

However, searching for item.text didn't look like there is anything that would break in that case.

@huumn
Copy link
Member

huumn commented Oct 17, 2023

I agree that this would ideally not require special treatment and simply occupy the text column on the post.

@rleed
Copy link
Contributor Author

rleed commented Oct 17, 2023

Great idea to simplify with the text field... I will pursue that!

@rleed
Copy link
Contributor Author

rleed commented Oct 17, 2023

I think this is ready now.

@rleed rleed force-pushed the link-comment branch 2 times, most recently from 02f50f1 to 49b57af Compare October 20, 2023 13:53
@huumn huumn merged commit 92c5303 into stackernews:master Oct 22, 2023
1 check passed
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.

Add optional text input for link posts
3 participants