Fix UI and add setting options#3
Open
Leto-cmd wants to merge 2 commits into
Open
Conversation
TheGittyPerson
requested changes
Mar 27, 2026
Owner
TheGittyPerson
left a comment
There was a problem hiding this comment.
This works very nicely and I don't see any immediate issues. Although could you please move the settings button to the top right corner of the screen to avoid covering the text at the bottom? Thanks!
TheGittyPerson
requested changes
Mar 27, 2026
Owner
TheGittyPerson
left a comment
There was a problem hiding this comment.
Also, I not the most familiar with JS, so here's a brief review of possible issues with main.js, generated with the help of AI. Please let me know if these are valid or worth considering:
Findings
- High:
buttonandclickCountare queried at module top level and used immediately (button.addEventListener,button.getBoundingClientRect) without null checks. If the script loads before the DOM is ready or on a page without those elements, this will throw and break the page. Move the queries insideDOMContentLoaded, usedefer, or guard for null before using. - High:
initSettingsassumes#settings-toggle,#settings-modal,#settings-backdrop, and#settings-closealways exist. Any missing element will cause a crash when adding listeners or manipulating classes. Guard those elements or short-circuit if any are missing. - Medium:
createFloatingMessageonly removes the element ontransitionend. If the CSS transition is removed or user prefers reduced motion and the transition is disabled, the element may never be removed, leading to DOM growth over time. Add a fallback timeout to remove the element. - Low: On very small viewports,
maxOffsetX/maxOffsetYcan become negative, which makes the random positioning math awkward. Clamp them toMath.max(0, …)to avoid inverted ranges.
Open Questions / Assumptions
- Is this script loaded with defer or at the end of
bodyon all pages that include it? If not, the null access issues will occur. - Are the settings elements guaranteed to exist on every page using this script, or should the logic be optional?
Signed-off-by: Morpheus <167074500+thegittyperson@users.noreply.github.com>
Owner
|
@Leto-cmd so are you planning on... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I've noticed that the text sometimes overlaps with the button, so it is aesthetically unpleasant, so I have changed that. I have also added "feminine" and "neutral" options to the text that comes out, so that a larger audience can use the tool, along with a dark/light mode.
lmk what you think.
Leto