Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

#1422 Textarea enable linebreak (submitOnEnter) #1517

Merged
merged 6 commits into from
Jul 22, 2021

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented Jul 21, 2021

Description

See #1422

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation added/updated

@update-docs
Copy link

update-docs bot commented Jul 21, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Please add a changelog item in changelog/unreleased/, linking to the issue this is addressing (if applicable)

Also, please run yarn lint:eslint --fix to fix linter issues locally (they're failing in CI currently) ;)

@lookacat
Copy link
Contributor Author

@pascalwengerter yes sorry only wanted to create a WIP PR ^^ but ofc you are right I fixed it

@pascalwengerter
Copy link
Contributor

@pascalwengerter yes sorry only wanted to create a WIP PR ^^ but ofc you are right I fixed it

No worries, was under the impression it's ready to review since you requested it ;) good work, either way!

@lookacat lookacat changed the title WIP: #1422 textarea linebreak #1422 textarea linebreak Jul 21, 2021
@lookacat lookacat changed the title #1422 textarea linebreak #1422 Textarea enable linebreak (submitOnEnter) Jul 21, 2021
src/components/OcTextarea.vue Outdated Show resolved Hide resolved
src/components/OcTextarea.vue Outdated Show resolved Hide resolved
@@ -158,9 +168,10 @@ export default {
this.$emit("focus", value)
},
onKeyDown(e) {
if (e.keyCode === 13) {
if (this.submitOnEnter && e.keyCode === 13 && !e.ctrlKey && !e.shiftKey) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.submitOnEnter && e.keyCode === 13 && !e.ctrlKey && !e.shiftKey) {
if (this.submitOnEnter && e.enterKey && !e.ctrlKey && !e.shiftKey) {

I like that you're using named keys. Please also change the keyCode === 13 to the named enterKey then for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sadly keyenter doesn't exist on the event, but I created a mini workaround to make it more readable

@lookacat
Copy link
Contributor Author

All issues mentioned should be resolved

@@ -158,9 +168,11 @@ export default {
this.$emit("focus", value)
},
onKeyDown(e) {
if (e.keyCode === 13) {
e.keyEnter = e.key === "Enter";
Copy link
Member

@kulmann kulmann Jul 22, 2021

Choose a reason for hiding this comment

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

Hm.... personally in favor of not altering the event. Since e.enterKey doesn't exist, you could just create a const and use it in the if statement, that still improves the readability a bit.
I.e. like this:

const enterKey = e.key === "Enter"
if (this.submitOnEnter && enterKey && !e.ctrlKey && !e.shiftKey) {

Copy link
Contributor Author

@lookacat lookacat Jul 22, 2021

Choose a reason for hiding this comment

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

looks fine to me ^^

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Would be superb to have working examples of both configs, but changes in general LGTM and props for adding tests! 💪🏽

@kulmann kulmann merged commit cc28d40 into master Jul 22, 2021
@pascalwengerter pascalwengerter deleted the #1422-textarea-linebreak branch July 23, 2021 13:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OcTextArea: Add a way to enter a line break
3 participants