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

Use username as a placeholder in the comment box if no other name is available #813

Closed
wants to merge 1 commit into from

Conversation

armish
Copy link
Member

@armish armish commented Aug 20, 2015

If user is logged in, then we have a guess about what author name s/he would like to use when commenting. This change enables suggesting username as a default author name when commenting. If user replaces this with another custom name, then that becomes the default.

Here is how this behaves (note that I am logged in as Arman):
screen recording 2015-08-20 at 06 15 pm

Review on Reviewable

@hammer
Copy link
Member

hammer commented Aug 20, 2015

Hmm why even make it editable if they have a username?

@armish
Copy link
Member Author

armish commented Aug 20, 2015

I agree that they shouldn't be editable; but our current schema doesn't seem to allow that: https://github.com/hammerlab/cycledash/blob/master/schema.py#L47-L59

We ideally allow users to have a preferred display name to be used throughout the site and do not allow them to enter arbitrary text for author name when commenting. However, that requires a relatively more thorough refactoring and this is just a quick hack to encourage the use of usernames for now.

I will create a separate issue to improve the way we handle user <-> comment relationships.

@ihodes
Copy link
Member

ihodes commented Aug 21, 2015

I think using the actually-logged-in user is actually an easier change that what you're having to do here. Instead of messing with the HTML/React code, you can just remove the Username field on the front-end, and on the backend just set the userId of the user who is commenting. This would require a migration (using Alembic, yay!)

@armish
Copy link
Member Author

armish commented Aug 21, 2015

👍

Non-editable user ids and migration of the database, it is then! I will ditch this PR when I am done with the new one.

@ihodes
Copy link
Member

ihodes commented Aug 21, 2015

Awesome, thanks! If you run into any troubles with the migration, let me know.

On Fri, Aug 21, 2015 at 11:29 PM, B. Arman Aksoy notifications@github.com
wrote:

👍

Non-editable user ids and migration of the database, it is then! I will ditch this PR when I am done with the new one.

Reply to this email directly or view it on GitHub:
#813 (comment)

@armish
Copy link
Member Author

armish commented Sep 3, 2015

closing this PR -- #831 does a better job than this one.

@armish armish closed this Sep 3, 2015
@ihodes ihodes deleted the suggest-username-as-default-author branch December 23, 2015 16:54
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.

3 participants