Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Initial Implementation of Slide to Confirm #689
Initial Implementation of Slide to Confirm #689
Changes from all commits
ddb98c7
a98e8a7
1786cac
9b0a25c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this logic is fine and we can proceed with it, I have a suggestion of a slight different one that seems to be less intrusive in the code:
This way we don't have to pass the whole success code (in this case it's only
mainVehicle.value.arm()
, but on more complex cases could be several lines of code) to the confirmation slider, but just to read its result. This way the code get's cleaner, more readable, with less indentations, and its more in line with what we currently use for SweetAlert2 confirmation dialogs, for example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also makes sense. I will change. Thanks for all the great feedback!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to do this. let me know if you would like me to make that change. If I do, do you think I should reset that commit make the change and recommit, or would it be better to make a separate commit to make that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cool, but at the same time it would be an improvement, so you can add it to a following commit, so you don't have to recommit everything, or we can settle with how it is right now and allow this part to be improved on a future PR, without hurries. I'm fine with both, so let me know what you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After giving it another look, I think it would take me a little bit of time to refactor.
If you are ok with it I think I would like to leave it how it is for now. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem!
I will test it tomorrow morning so we can merge.