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

The enter key will be enabled depending on the value of `InputContext… #11

Merged

Conversation

EddyTheCo
Copy link
Contributor

The enter key will be enabled depending on the value of InputContext.inputItem.EnterKeyAction.enabled.
This should fix #10

Copy link
Contributor

@AndreaRicchi AndreaRicchi left a comment

Choose a reason for hiding this comment

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

LGTM but fix the formatting.

@@ -9,6 +9,6 @@ Key {
btnText: "\n"
btnDisplayedText: InputPanel.enterIcon === "" ? "Enter" : ""
btnIcon: InputPanel.enterIcon === "" ? "" : InputPanel.enterIcon
enabled: InputContext.inputItem?.EnterKeyAction.enabled ?? true
enabled: (InputContext.inputItem)?InputContext.inputItem.EnterKeyAction.enabled : true
Copy link
Contributor

Choose a reason for hiding this comment

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

Format the code:

Suggested change
enabled: (InputContext.inputItem)?InputContext.inputItem.EnterKeyAction.enabled : true
enabled: (InputContext.inputItem) ? InputContext.inputItem.EnterKeyAction.enabled : true

Copy link
Contributor

@AndreaRicchi AndreaRicchi left a comment

Choose a reason for hiding this comment

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

Squash all the commits into a single one for better readability.

@@ -9,6 +9,6 @@ Key {
btnText: "\n"
btnDisplayedText: InputPanel.enterIcon === "" ? "Enter" : ""
btnIcon: InputPanel.enterIcon === "" ? "" : InputPanel.enterIcon
enabled: InputContext.inputItem?.EnterKeyAction.enabled ?? true
enabled: InputContext.inputItem ? InputContext.inputItem.EnterKeyAction.enabled : true
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the empty space at the end

@EddyTheCo
Copy link
Contributor Author

Squash all the commits into a single one for better readability
Do I need to open a new PR to do that?
I think you can do it yourself when merging this PR.
See picture
Screenshot from 2024-04-07 09-44-47

@EddyTheCo EddyTheCo force-pushed the fix_runtime_error_from_qml_typo branch from d659506 to 321702f Compare April 8, 2024 06:59
@EddyTheCo
Copy link
Contributor Author

I have squashed all the commits of my branch.
As I understand from here this should be avoided.

Don't rebase public history
you should never rebase commits once they've been pushed to a public repository. The rebase would replace the old commits with new ones and it would look like that part of your project history abruptly vanished.

Perhaps a squash and merge from your side would be a better workflow.

The enter key will be enabled depending on the value of `InputContext.inputItem.EnterKeyAction.enabled`.
@EddyTheCo EddyTheCo force-pushed the fix_runtime_error_from_qml_typo branch from 321702f to 970d070 Compare April 8, 2024 07:26
@AndreaRicchi
Copy link
Contributor

Having three commits for a single line change is bad in any scenario. Specifically formatting code that you changed must be amended in your first commit instead of having:

  • Changed A
  • Formatted A
  • Applied suggestions to A

It's a bad practice and should be squashed by the submitter

@AndreaRicchi AndreaRicchi merged commit 76db0aa into amarula:main Apr 8, 2024
1 check passed
@EddyTheCo EddyTheCo deleted the fix_runtime_error_from_qml_typo branch April 8, 2024 08:25
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.

Runtime error from qml typo?
2 participants