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

Simplify Jump Options #3683

Merged
merged 7 commits into from
Oct 11, 2024
Merged

Simplify Jump Options #3683

merged 7 commits into from
Oct 11, 2024

Conversation

aslassi777
Copy link
Collaborator

@aslassi777 aslassi777 commented Oct 3, 2024

Resolves #3676

Removed first page of the jump modal and now it only displays the second page automatically. Also adjusted the text to help users understand that if they are stuck, they should use the "stuck" button instead of jump. Also cleaned/reorganized the code, added some comments to help clarify things.

Edit: Also adjusted code and PR in regards to comments you made at the original PR. I updated the PR title, shortened the length of the sentence, added translations, and removed logs, functions (and event bindings related to those functions) we don't need anymore (removed cancel first, ClickUnavailable, clickExplore) since we removed the first jump page. Didn't update or add any logs, so no changes made to wiki page you mentioned.

Also removed html of the unused buttons and "first box", and also refactored UIModal code and relevant CSS to match. Also adjusted naming of cancelation function, and removed unnecessary HTML calls in main as well as unnecessary translations in NZ file.

Before/After screenshots (if applicable)

Before:
Screenshot 2024-10-03 at 4 59 07 PM

After:
Screenshot 2024-10-09 at 3 47 15 PM

Things to check before submitting the PR
  • I've written a descriptive PR title.
  • I've added/updated comments for large or confusing blocks of code.
  • I've included before/after screenshots above.
  • I've asked for and included translations for any user facing text that was added or modified.
  • I've updated any logging. Clicks, keyboard presses, and other user interactions should be logged. If you're not sure how (or if you need to update the logging), ask Mikey. Then make sure the documentation on this wiki page is up to date for the logs you added/updated.

@misaugstad
Copy link
Member

I couldn't find any logging related to jumping

That's from lines like this: tracker.push('ModalSkip_ClickJump');

A few more things:

  1. Can you edit your PR title to something more descriptive? Ideally commit messages should be more descriptive as well! Something like "Simplifying Jump button options" works well enough! Ideally I want to be able to look through a list of PRs in the future and find any that might be relevant to a problem I'm having. And a title like "Latest changes for this fix" doesn't help too much 😂
  2. Now that I see the text in your screenshot, it looks a bit lengthy! Maybe let's shorten it to something like:

    If you can't move forward, click the "Stuck" button instead!

  3. Make sure to make the changes for all languages, not just English! Instructions in this wiki page
  4. Make sure to finish cleaning up by removing anything that is no longer used! Any time you delete something, work hard to remove all the stuff that no longer has a use! Just because they're not on the screen doesn't mean that they aren't mucking up our code somewhere!

@misaugstad
Copy link
Member

Oh and I updated your checklist of things for the PR, you checked a bunch of boxes that weren't actually done! 😉

@aslassi777
Copy link
Collaborator Author

aslassi777 commented Oct 4, 2024

Sorry @misaugstad! Will fix it up over the weekend. Sorry about the title 😭 - I think it just took my commit message when I pushed, I forgot to change it. Yes, will change for all languages (that was very helpful), I'll change the text, and I'll parse the code thoroughly for stuff that isn't used/logging related issues. I'll come back with a more thorough pr after the weekend. I will make sure to be more thorough in the PR template and also I will change the wiki appropriately!

@aslassi777 aslassi777 changed the title Latest changes for this fix Simplify Jump Options Oct 4, 2024
@misaugstad
Copy link
Member

Thanks @aslassi777 and no worries, it's all part of the learning process!! Just looking for improvement going forward, and hoping that you have a better idea of the attention to detail we're looking for!

@aslassi777
Copy link
Collaborator Author

thanks @misaugstad, appreciate it... got started on the pull request again, hope to have it done this week!

…ed uneccessary logs and functions for the first jump page that we removed
…e removed (such as click unavailable, click explore, and cancel first)
@aslassi777
Copy link
Collaborator Author

aslassi777 commented Oct 9, 2024

@misaugstad made relevant changes to PR. Please tell me if everything looks good, if not I can promptly fix. I updated my original comment!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

So there's still a bunch of stuff that's no longer being used that can be removed! The two buttons that we're no longer using: the HTML hasn't been removed, neither has the translations for the text in those buttons, nor the reference to the HTML ID in Main.js.

PR is looking more clean though, def an improvement!

@aslassi777
Copy link
Collaborator Author

aslassi777 commented Oct 10, 2024

Yep! That all makes sense! Wasn't 100 percent sure the translations automatically default to plain english, that's very helpful. I'll go ahead and rename. Renaming makes sense. I did not look at main.js but I will go through that now and remove anything that I see, along with removing the translations for the buttons. Thanks again for pointers @misaugstad, I'll have another PR shortly

…actored to first box only, and made changes to main and css to reflect refactor in regards to function IDs that were referenced in main and relevant firstbox changes
@aslassi777
Copy link
Collaborator Author

@misaugstad I think I've fixed everything! very sorry for all of the changes, still learning how everything works together in the code base, your reviews have been very helpful

… css accordingly to match removal of the html
Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Okay, still just working on renaming things to wipe away the history that we used to have :) Looks like almost everything that needed to be deleted is gone though!

…e html, renaming the cancel button, and ensuring any traces of all code, including first box and second box have now been removed
@aslassi777
Copy link
Collaborator Author

aslassi777 commented Oct 11, 2024

@misaugstad all changes are fixed and I took another look. I think we are good to go now!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Alright, nicely done!! I made a tiny commit to do a final bit of cleanup, but the only thing there to note for you is that you accidentally removed #modal-comment-box from the CSS file, which messed up the "Feedback" button UI.

Other than that, all I did was reorder the lines in Main.js to match the structure of the HTML, and I added a jquery object for #modal-skip-background so that it matched everything else.

Thanks for going through all the iteration, I'm really happy with how it looks now! Much more clean :)

@misaugstad misaugstad merged commit 3834ad2 into develop Oct 11, 2024
@misaugstad misaugstad deleted the 3676-fix-jump branch October 11, 2024 21:06
@aslassi777
Copy link
Collaborator Author

thank you for bearing with me @misaugstad! hoping it can only get better from here haha

@misaugstad misaugstad mentioned this pull request Oct 18, 2024
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.

Remove "cannot go in the direction you want me to walk" option from Jump button
2 participants