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

Refactor text formatting functions #1418

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marcustyphoon
Copy link
Collaborator

Description

Should result in no functional changes. This extracts some utility functions that are used in the modals of various scripts into a utility file to reduce repetition.

Open to bikeshedding (different file name, interface.js, modals.js, whatever).

Testing steps

Copy link
Owner

@AprilSylph AprilSylph left a comment

Choose a reason for hiding this comment

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

if you want bikeshedding... here's some extreme bikeshedding for you...!


I feel like trying to follow XKit 7's pattern of grouping utils just for the sake of grouping them was kinda silly, and I regret doing it. Rather than think up names for collections of utils which don't even share any code, I think the easier and clearer thing is to have one export per util file, e.g.

  • src/util/construct_relative_time_string.js
    • Defines thresholds
    • Defines relativeTimeFormat
    • Exports constructRelativeTimeString
  • src/util/date_time_format.js
    • Exports dateTimeFormat
  • src/util/elements_as_list.js
    • Exports elementsAsList

Of course, this pattern doesn't work for util files which export several interdependent values (like user_blogs.js), but at least that way we're only doing the group-naming work when it's actually necessary.

@marcustyphoon
Copy link
Collaborator Author

Spoken like a true developer-whose-project-at-work-uses-a-bundler, I kid, I kid, don't sue me, I'm innocent :P

I certainly don't think I would go unequivocally to the opposite extreme, where one winds up with a folder full of function names in alphabetical order. If a set of functions will usually be used together or would need all need to be refactored at the same time, I think it makes sense to be able to find one from the other via context. But yeah, I think there's certainly places for it. interface.js is pretty much a grab bag, iirc. I feel like it might make sense to put the functions that take an NPF object together (might have to add one to normalize community posts?)

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.

None yet

2 participants