feat: added touch/slide on prematch option buttons (#1529) #2517

Mandolinx · 2023-10-22T11:06:50Z

This fixes issue: #1529

created a function called buttonSlide in button.ts for pre-match button game options to touch and slide
created a new html class in index.ejs called "dragIt"
made all radio labels have class "dragIt"
added the buttonSlide function to script.ts

clicking on a pre-match button game option and dragging to the next button will click that next button and reset original button.

vercel · 2023-10-22T11:06:54Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
ancientbeast	✅ Ready (Inspect)	Visit Preview	Oct 24, 2023 5:02pm

ghost · 2023-10-22T11:08:07Z

👇 Click on the image for a new way to code review

Legend

DreadKnight

Heya! Some things that should probably be poked.

DreadKnight · 2023-10-22T13:53:46Z

src/index.ejs

@@ -74,91 +74,121 @@
 									<h2>Player Mode&nbsp;<span id="playerModeType"></span></h2>
 									<div id="playerMode" class="typeRadio">
 										<input type="radio" id="p2" name="playerMode" value="2" checked>
-										<label for="p2">1 versus 1</label>
+										<label for ="p2" class="draggable-label">1 versus 1</label>


Space, the final frontier.

DreadKnight · 2023-10-22T13:55:02Z

src/index.ejs

 										<input type="radio" id="p4" name="playerMode" value="4">
-										<label for="p4">2 versus 2</label>
+										<label for="p4" class="draggable-label">2 versus 2</label>


Maybe class name should be way shorter, like "dragit" or so.

DreadKnight · 2023-10-22T13:57:00Z

src/index.ejs

 									</div>
 								</div>
+								<!--Script to add sliding to prematch option buttons-->
+								<script>


Best to make index file smaller (as we even plan to split it into files). Would be best if script was separate file in src/ui and be imported using webpack or whatever.

DreadKnight · 2023-10-22T13:57:50Z

src/index.ejs

+										  selectedRadio = radio.previousElementSibling;
+										});
+
+										// check new dragged radio/button


Comment style consistency: capitalized first letter, // Check...

DreadKnight · 2023-10-22T14:00:00Z

@Mandolinx I've tested this and works well, however, I've opened a review regarding the coding itself to polish things.

Mandolinx · 2023-10-24T07:38:14Z

Hi @DreadKnight I've just updated my code such that the script is now in a separate file in UI called "slider.js", and fixed various syntax and style issues. During my process, I was not familiar with webpack so I'm not entirely sure if my method of importing is suitable for this issue so that may be an area of focus in the review. Also, my script file that does the slide is in a separate file but it only contains a function, so I was wondering whether it'd be better to just add the function directly to script.ts or add the function to Button.ts. Thanks!

p.s. interface.js is said to be changed but all I did was copy and paced the interface file from repo because git pull was not working.

DreadKnight · 2023-10-24T15:45:47Z

Hi @DreadKnight I've just updated my code such that the script is now in a separate file in UI called "slider.js", and fixed various syntax and style issues. During my process, I was not familiar with webpack so I'm not entirely sure if my method of importing is suitable for this issue so that may be an area of focus in the review. Also, my script file that does the slide is in a separate file but it only contains a function, so I was wondering whether it'd be better to just add the function directly to script.ts or add the function to Button.ts. Thanks!

p.s. interface.js is said to be changed but all I did was copy and paced the interface file from repo because git pull was not working.

Moving the stuff in script.ts it doesn't make sense imo, because that's an initial set-up thingy, but it might make sense in button.ts if the file is about the pre-match screen buttons (wish it had some description), so feel free to move if so and also remove "Used for prematch button game options to slide" comment line if moving it there.

Regarding interface.js, you usually don't need to pull master to latest when it comes to PRs; you haven't touched that file in your commits and the copy-pasting that you did actually reverts a patch of mine that I might have missed. Just exclude your changes by reverting file to what it was ideally.

Mandolinx · 2023-10-24T17:06:48Z

Moving the stuff in script.ts it doesn't make sense imo, because that's an initial set-up thingy, but it might make sense in button.ts if the file is about the pre-match screen buttons (wish it had some description), so feel free to move if so and also remove "Used for prematch button game options to slide" comment line if moving it there.

Regarding interface.js, you usually don't need to pull master to latest when it comes to PRs; you haven't touched that file in your commits and the copy-pasting that you did actually reverts a patch of mine that I might have missed. Just exclude your changes by reverting file to what it was ideally.

@DreadKnight As requested, I reverted to my original version of interface and I put the buttonSlider into the button file. I also changed the function to use jquery since I was having problems earlier copy and pasting my old function into the file without it, but it has the same functionality so everything should be fine for that. Thanks!

DreadKnight · 2023-10-25T02:11:22Z

Moving the stuff in script.ts it doesn't make sense imo, because that's an initial set-up thingy, but it might make sense in button.ts if the file is about the pre-match screen buttons (wish it had some description), so feel free to move if so and also remove "Used for prematch button game options to slide" comment line if moving it there.
Regarding interface.js, you usually don't need to pull master to latest when it comes to PRs; you haven't touched that file in your commits and the copy-pasting that you did actually reverts a patch of mine that I might have missed. Just exclude your changes by reverting file to what it was ideally.

@DreadKnight As requested, I reverted to my original version of interface and I put the buttonSlider into the button file. I also changed the function to use jquery since I was having problems earlier copy and pasting my old function into the file without it, but it has the same functionality so everything should be fine for that. Thanks!

@Mandolinx Thanks way better, good job! 👍🏻

feat: added touch/slide on prematch option buttons

c0a4121

vercel bot deployed to Preview October 22, 2023 11:07 View deployment

DreadKnight requested changes Oct 22, 2023

View reviewed changes

DreadKnight marked this pull request as draft October 22, 2023 14:00

created slider file and fixed syntax errors in index.ejs

50984dc

vercel bot deployed to Preview October 24, 2023 07:25 View deployment

fixed interface file

67a4c14

vercel bot deployed to Preview October 24, 2023 07:32 View deployment

Mandolinx marked this pull request as ready for review October 24, 2023 07:38

moved buttonSlider to button.ts

bf19042

vercel bot deployed to Preview October 24, 2023 16:58 View deployment

added some comments to function

5305725

vercel bot deployed to Preview October 24, 2023 17:01 View deployment

added some comments to function

28d985f

vercel bot deployed to Preview October 24, 2023 17:02 View deployment

DreadKnight merged commit 0e6b305 into FreezingMoon:master Oct 25, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added touch/slide on prematch option buttons (#1529) #2517

feat: added touch/slide on prematch option buttons (#1529) #2517

Mandolinx commented Oct 22, 2023 •

edited

Loading

vercel bot commented Oct 22, 2023 •

edited

Loading

ghost commented Oct 22, 2023 •

edited by ghost

Loading

Legend

DreadKnight left a comment

DreadKnight Oct 22, 2023

DreadKnight Oct 22, 2023

DreadKnight Oct 22, 2023

DreadKnight Oct 22, 2023

DreadKnight commented Oct 22, 2023 •

edited

Loading

Mandolinx commented Oct 24, 2023 •

edited

Loading

DreadKnight commented Oct 24, 2023

Mandolinx commented Oct 24, 2023 •

edited

Loading

DreadKnight commented Oct 25, 2023

feat: added touch/slide on prematch option buttons (#1529) #2517

feat: added touch/slide on prematch option buttons (#1529) #2517

Conversation

Mandolinx commented Oct 22, 2023 • edited Loading

vercel bot commented Oct 22, 2023 • edited Loading

ghost commented Oct 22, 2023 • edited by ghost Loading

Legend

DreadKnight left a comment

Choose a reason for hiding this comment

DreadKnight Oct 22, 2023

Choose a reason for hiding this comment

DreadKnight Oct 22, 2023

Choose a reason for hiding this comment

DreadKnight Oct 22, 2023

Choose a reason for hiding this comment

DreadKnight Oct 22, 2023

Choose a reason for hiding this comment

DreadKnight commented Oct 22, 2023 • edited Loading

Mandolinx commented Oct 24, 2023 • edited Loading

DreadKnight commented Oct 24, 2023

Mandolinx commented Oct 24, 2023 • edited Loading

DreadKnight commented Oct 25, 2023

Mandolinx commented Oct 22, 2023 •

edited

Loading

vercel bot commented Oct 22, 2023 •

edited

Loading

ghost commented Oct 22, 2023 •

edited by ghost

Loading

DreadKnight commented Oct 22, 2023 •

edited

Loading

Mandolinx commented Oct 24, 2023 •

edited

Loading

Mandolinx commented Oct 24, 2023 •

edited

Loading