Initial Implementation of Slide to Confirm #689

ericjohnson97 · 2024-01-18T02:48:13Z

2024-01-17_20-38-16.mp4

Closes #578

src/App.vue

package.json

src/stores/mainVehicle.ts

rafaellehmkuhl · 2024-01-18T17:01:46Z

src/stores/mainVehicle.ts

-  function arm(): void {
-    mainVehicle.value?.arm()
+  function arm(): Promise<void> {
+    return slideToConfirm(() => {
+      if (!mainVehicle.value) {
+        throw new Error('action rejected or failed')
+      }
+      mainVehicle.value.arm()
+    })


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:

async function arm(): Promise<void> { if (!mainVehicle.value) throw new Error('No vehicle available to be armed.') const slideConfirmed = await slideToConfirm('Slide to arm', 10000) if (slideConfirmed) { mainVehicle.value.arm() return } console.error('No confirmation for arming action received') }

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.

this also makes sense. I will change. Thanks for all the great feedback!

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?

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.

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. 🙂

No problem!
I will test it tomorrow morning so we can merge.

rafaellehmkuhl · 2024-01-18T17:06:42Z

I really liked the new functionality Eric!

UI/UX speaking, I think additions like the transition for it to appear/disappear or the color change (suggested by @danielhonies) would be great, although they shouldn't stop us from merging this PR.

PS: I took the liberty to append your screen recording to this PR, as a visual aid.

ericjohnson97 · 2024-01-19T00:49:42Z

I really liked the new functionality Eric!

UI/UX speaking, I think additions like the transition for it to appear/disappear or the color change (suggested by @danielhonies) would be great, although they shouldn't stop us from merging this PR.

PS: I took the liberty to append your screen recording to this PR, as a visual aid.

Thanks for adding the video. They will be good for people viewing the PR to see.

showing the slider green for a second would be a easy addition because the slider is this component and it has that by default, but right now the slider stops showing too fast to see it.
https://github.com/joseph2/vue-slide-unlock

related to this, I see that the pipeline is failing because of typing related to adding that component and I don't know how to fix it. Do you have any thoughts?

rafaellehmkuhl · 2024-01-19T14:26:40Z

I really liked the new functionality Eric!
UI/UX speaking, I think additions like the transition for it to appear/disappear or the color change (suggested by @danielhonies) would be great, although they shouldn't stop us from merging this PR.
PS: I took the liberty to append your screen recording to this PR, as a visual aid.

Thanks for adding the video. They will be good for people viewing the PR to see.

showing the slider green for a second would be a easy addition because the slider is this component and it has that by default, but right now the slider stops showing too fast to see it. https://github.com/joseph2/vue-slide-unlock

related to this, I see that the pipeline is failing because of typing related to adding that component and I don't know how to fix it. Do you have any thoughts?

The linter is blaming that there's no type declarations for one of the libraries you added ('vue-drag-verify'), but I'm seeing here that you are importing it but not using. You can just remove its code from the main.ts file, bun remove vue-drag-verify, bun install and then push the branch again to see if it fixes the problem.

ericjohnson97 · 2024-01-26T00:52:47Z

I really liked the new functionality Eric!
UI/UX speaking, I think additions like the transition for it to appear/disappear or the color change (suggested by @danielhonies) would be great, although they shouldn't stop us from merging this PR.
PS: I took the liberty to append your screen recording to this PR, as a visual aid.

Thanks for adding the video. They will be good for people viewing the PR to see.
showing the slider green for a second would be a easy addition because the slider is this component and it has that by default, but right now the slider stops showing too fast to see it. https://github.com/joseph2/vue-slide-unlock
related to this, I see that the pipeline is failing because of typing related to adding that component and I don't know how to fix it. Do you have any thoughts?

The linter is blaming that there's no type declarations for one of the libraries you added ('vue-drag-verify'), but I'm seeing here that you are importing it but not using. You can just remove its code from the main.ts file, bun remove vue-drag-verify, bun install and then push the branch again to see if it fixes the problem.

You are totally right. that was a package I was trying to use before I settled on vue-slide-unlock

rafaellehmkuhl

I've tested here and it works like a charm!

@ericjohnson97 could you just reword the 3rd commit, from "widgets" to "components" before we merge?

ericjohnson97 · 2024-01-26T15:24:44Z

I've tested here and it works like a charm!

@ericjohnson97 could you just reword the 3rd commit, from "widgets" to "components" before we merge?

It took me a couple tries, but I think I got it now. 🙂

rafaellehmkuhl · 2024-01-26T16:25:16Z

I've tested here and it works like a charm!
@ericjohnson97 could you just reword the 3rd commit, from "widgets" to "components" before we merge?

It took me a couple tries, but I think I got it now. 🙂

Cool!

You can usually use git rebase HEAD~3 -i for that (change 3 with the proper number of commits). Then put a "reword" instead of "pick" on the line you want to change the commit message. I think that's the easiest way.

ericjohnson97 · 2024-01-26T16:31:15Z

I've tested here and it works like a charm!
@ericjohnson97 could you just reword the 3rd commit, from "widgets" to "components" before we merge?

It took me a couple tries, but I think I got it now. 🙂

Cool!

You can usually use git rebase HEAD~3 -i for that (change 3 with the proper number of commits). Then put a "reword" instead of "pick" on the line you want to change the commit message. I think that's the easiest way.

Yep! I figured it out. I just had never done that before.

Thanks for all your advice with the design and all your help with the review!

rafaellehmkuhl · 2024-01-26T16:33:14Z

Yep! I figured it out. I just had never done that before.

Thanks for all your advice with the design and all your help with the review!

No problem. Thanks again for the contribution, Eric.
And feel free to ask for help whenever needed :)

rafaellehmkuhl requested changes Jan 18, 2024

View reviewed changes

ericjohnson97 added 2 commits January 25, 2024 16:26

package: updated floating-vue

ddb98c7

package: added vue-slide-unlock

a98e8a7

ericjohnson97 force-pushed the feature/slide_to_confirm branch from 25456dd to 4f4e594 Compare January 26, 2024 00:45

ericjohnson97 marked this pull request as ready for review January 26, 2024 00:54

rafaellehmkuhl self-requested a review January 26, 2024 01:27

rafaellehmkuhl approved these changes Jan 26, 2024

View reviewed changes

ericjohnson97 force-pushed the feature/slide_to_confirm branch 2 times, most recently from 1d17e39 to 4f35a3b Compare January 26, 2024 15:21

ericjohnson97 added 2 commits January 26, 2024 07:22

components: added slide to confirm component

1786cac

slide-to-confirm: added slide to confirm functionality

9b0a25c

ericjohnson97 force-pushed the feature/slide_to_confirm branch from 4f35a3b to 9b0a25c Compare January 26, 2024 15:23

rafaellehmkuhl merged commit 510e60f into bluerobotics:master Jan 26, 2024
7 checks passed

rafaellehmkuhl added the docs-needed Change needs to be documented label Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Implementation of Slide to Confirm #689

Initial Implementation of Slide to Confirm #689

ericjohnson97 commented Jan 18, 2024 •

edited by rafaellehmkuhl

Loading

rafaellehmkuhl Jan 18, 2024

ericjohnson97 Jan 19, 2024

ericjohnson97 Jan 26, 2024

rafaellehmkuhl Jan 26, 2024

ericjohnson97 Jan 26, 2024

rafaellehmkuhl Jan 26, 2024

rafaellehmkuhl commented Jan 18, 2024 •

edited

Loading

ericjohnson97 commented Jan 19, 2024

rafaellehmkuhl commented Jan 19, 2024

ericjohnson97 commented Jan 26, 2024

rafaellehmkuhl left a comment

ericjohnson97 commented Jan 26, 2024

rafaellehmkuhl commented Jan 26, 2024 •

edited

Loading

ericjohnson97 commented Jan 26, 2024

rafaellehmkuhl commented Jan 26, 2024

Initial Implementation of Slide to Confirm #689

Initial Implementation of Slide to Confirm #689

Conversation

ericjohnson97 commented Jan 18, 2024 • edited by rafaellehmkuhl Loading

rafaellehmkuhl Jan 18, 2024

Choose a reason for hiding this comment

ericjohnson97 Jan 19, 2024

Choose a reason for hiding this comment

ericjohnson97 Jan 26, 2024

Choose a reason for hiding this comment

rafaellehmkuhl Jan 26, 2024

Choose a reason for hiding this comment

ericjohnson97 Jan 26, 2024

Choose a reason for hiding this comment

rafaellehmkuhl Jan 26, 2024

Choose a reason for hiding this comment

rafaellehmkuhl commented Jan 18, 2024 • edited Loading

ericjohnson97 commented Jan 19, 2024

rafaellehmkuhl commented Jan 19, 2024

ericjohnson97 commented Jan 26, 2024

rafaellehmkuhl left a comment

Choose a reason for hiding this comment

ericjohnson97 commented Jan 26, 2024

rafaellehmkuhl commented Jan 26, 2024 • edited Loading

ericjohnson97 commented Jan 26, 2024

rafaellehmkuhl commented Jan 26, 2024

ericjohnson97 commented Jan 18, 2024 •

edited by rafaellehmkuhl

Loading

rafaellehmkuhl commented Jan 18, 2024 •

edited

Loading

rafaellehmkuhl commented Jan 26, 2024 •

edited

Loading