Skip to content

General fixes#1

Open
loucyx wants to merge 8 commits intoEnichan:mainfrom
loucyx:main
Open

General fixes#1
loucyx wants to merge 8 commits intoEnichan:mainfrom
loucyx:main

Conversation

@loucyx
Copy link
Copy Markdown

@loucyx loucyx commented Oct 6, 2025

I took the liberty of going over the code in the FediTag class, and adding this fixes/features:

  • Remove all _this = this: Those aren't necessary because you're using arrow functions that don't have their own context, so is safe to just use this.
  • Add JSDocs: With JSDocs we get better auto-completion and type-checking making it easier to use and safer.
  • Safety changes: In JS is heavily discouraged to use == and != an instead is recommended to always use === and !==.
  • Remove document.getElementById: Saved the reference to the elements in class properties and use those references instead. This also would remove the need for id (I didn't made this change myself, but I recommend it to avoid id collision).
  • Remove constructor setup: You can do the initial setup of properties when declaring them directly on the body of the class.
  • Remove all for: Usages of for could be easily replaced with Array methods such as forEach, some, reduce and more.

There's a bunch more improvements that could be done, happy to do those if you like these! 😊

@Enichan
Copy link
Copy Markdown
Owner

Enichan commented Oct 8, 2025

Apparently github doesn't notify me about pull requests so that's cool. Anyway I'll look at this hopefully soon when I'm not being flattened by constant migraines since there's a lot.

@loucyx
Copy link
Copy Markdown
Author

loucyx commented Oct 9, 2025

@Enichan no rush at all! And feel free to ask about any change, I'll gladly explain it in detail!

@Enichan
Copy link
Copy Markdown
Owner

Enichan commented Oct 31, 2025

So most of this looks good to me, although I'm a bit crusty so going 😒 at all the newfangled iterators, but that's a me problem. The big change I'd like to see is in regards to if statements, where I tend to prefer the "if" path to be the exceptional path, and the "else" path to be the default expected path, and I noticed this got reversed in some spots.

@loucyx
Copy link
Copy Markdown
Author

loucyx commented Nov 3, 2025

So most of this looks good to me, although I'm a bit crusty so going 😒 at all the newfangled iterators, but that's a me problem. The big change I'd like to see is in regards to if statements, where I tend to prefer the "if" path to be the exceptional path, and the "else" path to be the default expected path, and I noticed this got reversed in some spots.

Oh that's ok, I can reverse those. I just followed the common practice of avoid negated conditions. I'll update the PR later today 😊

Comment on lines +105 to +109
if (!isOnlyHashtags) {
return;
}

contents.removeChild(para);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It ends up being more code to do the same thing:

if (!a) { return; }
b();

// vs

if (a) { b() }

... but I just updated the code to have the exceptional paths as requested.

@loucyx
Copy link
Copy Markdown
Author

loucyx commented Nov 9, 2025

Sorry for the delay @Enichan! It took a little bit longer because I didn't had the bandwidth to also deal with merging the new main (which has the altText sanitization on it). I addressed your concern with the negated ifs, lmk about any other concerns! I'm happy to address them!

@Enichan
Copy link
Copy Markdown
Owner

Enichan commented Nov 13, 2025

No worries about the delay, I was sick so I'm still trying to catch up on stuff

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.

2 participants