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: Make it possible to close inline reply with the escape key. #2273

Merged
merged 2 commits into from
Dec 2, 2022

Conversation

jcsteh
Copy link
Collaborator

@jcsteh jcsteh commented Nov 28, 2022

Fixes #915.

this.reply()
try {
// return focus to the article.
this.refs.node.closest('article').focus({ preventScroll: true })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. There's some probably not great coupling here, but I'm not sure how else to get at the article.
  2. This is potentially controversial. I think most users who value this will be using keyboard shortcuts, in which case focusing the article makes sense, since they probably opened the reply by focusing the article and pressing r. On the other hand, a decision was made to focus the Reply button after a reply is posted. Personally, I think the article should be focused in that case too, but I can understand the other perspective here too; the reply may well have been opened by tabbing to the reply button and pressing it.

Copy link
Owner

Choose a reason for hiding this comment

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

There's some probably not great coupling here, but I'm not sure how else to get at the article.

One common way to do this in Svelte is to fire an event and have the parent handle it. I can refactor to do that.

I don't think this is super controversial. I'm not sure what else Esc could be used for. Also, it is possible to disable keyboard shortcuts entirely.

BTW I also have a utility called tryToFocusElement that does the try/catch.

Copy link
Owner

Choose a reason for hiding this comment

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

Done! ccdd54f

@nolanlawson nolanlawson merged commit 8154381 into nolanlawson:master Dec 2, 2022
alice-werefox pushed a commit to alice-werefox/sema-werefox-cafe that referenced this pull request Apr 3, 2023
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.

No way to close inline reply with keyboard shortcuts
2 participants