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

Nuclear refactor #74

Open
wants to merge 53 commits into
base: development
Choose a base branch
from
Open

Nuclear refactor #74

wants to merge 53 commits into from

Conversation

kcargile
Copy link
Collaborator

@kcargile kcargile commented Sep 10, 2024

This one is pretty big and will need a good bit of testing before it goes to NPM, but incorporates most of the changes I've been using on some of my own websites.

This is a major version bump and contains breaking changes.

Here's what changed:

  • Refactored to Typescript
  • Renamed props, vars, and methods for clarity
  • Added debug output support (fixes Output undefined textblock definitions to console in debug mode (debug mode tbd) #47, fixes Add debug mode #57)
  • Added a debounce method for the resize() event handler to improve performance
  • Return event cancelation handles to address memory leaks in SPAs (relevant to Import Textblock for React #37)
  • Added configuration props (debug and debounce) with some reasonable defaults
  • Removed support for legacy parameter names
  • Removed support for ancient IE versions (many of those methods are no longer in the web API)
  • Adjust Husky config
  • Adjust prettier config
  • Adjust eslint config
  • Introduce Changesets for versioning (@theorosendorf I also added the CS bot to the repo)
  • Add some settings for folks using VSCode
  • Replace grunt with webpack
  • Exclude build artifacts from repo; output dir is now ./dist (npm run build)
  • Output TS type definitions and sourcemap
  • Target CommonJS
  • Bumped dependency versions (@theorosendorf I also closed the stale dependabot PRs)
  • Added a Dependabot build file for dependency PR grouping
  • Moved demo files out of the source root (now in /demo)
  • Pointed the demo to @latest on unpkg
  • Introduce SECURITY.MD
  • Update param names in the demo HTML
  • Added JSDocs (ChatGPT generated)
  • Miscellaneous housekeeping
  • Added link to textblock.io; fixes Add note to readme about seeing it in use at textblock.io #27
  • Readme updates
  • Introduce basic GHA build script
  • Inject version number into build output; fixes Echo version number to textblock.min.js? #31
  • Version bump

A few important notes:

I added filtering to remove non-HTML (HTMLElement) DOM nodes. This will prevent resizing other element types, e.g. SVGs, that don't share a common (styling) API. Not sure if this is desirable.

I added the following guard:

function calculateElementWidth(element?: HTMLElement | ParentNode | null) {
    if (!element) return 0;
    // ...

This seems like an edge case, but not sure if returning 0 is desirable.

The configuration and instrumentation options are pretty basic. There may be more debugging output that's relevant. It might also be desirable to add an optional config file (e.g. textblock.config.js) to set these and provide the ability to override defaults in a later version.

Updates to the README are coming in a subsequent commit.

Finally, I wasn't able to test publishing to NPM using the new build configuration. I propose that we automate this process via GHA on merge to the mainline. Happy to submit a PR for this later if it's useful.

Copy link

changeset-bot bot commented Sep 10, 2024

⚠️ No Changeset found

Latest commit: 69ec9de

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kcargile
Copy link
Collaborator Author

@theorosendorf Before this gets merged (assuming it will), if you want to push an RC package to NPM, I'll test it out in production on GRF.

@kcargile
Copy link
Collaborator Author

If anyone wants to try out the RC package, it's here:

https://www.npmjs.com/package/textblock/v/1.0.2

or

npm i textblock@rc

@theorosendorf
Copy link
Collaborator

Hey @kcargile — Just to confirm, is the file being called in the demo via unpkg, the minified version of this refactor?

https://unpkg.com/textblock/textblock.min.js

demo/index.html

@theorosendorf
Copy link
Collaborator

@kcargile — I'd like to change "Variable Grade" to "Variable Weight" because when we made this, variable fonts were new and no one was actually doing graded weights. The script actually adjusts weight so we should call it what it is.

Then later, we can add additional variable font features.

Also, for the sake of readability and usability, I think it would be best if we semi_SnakeCase the parameter names. Perhaps like fontSize_MinWidth or some variation of capitalization (just maybe not starting with a capital letter?). They're more scannable this way. And like camelCase, they remain double clickable for selecting them (kabab-case eludes double clicking).

All of this is just find and replace, right?

@kcargile
Copy link
Collaborator Author

kcargile commented Sep 30, 2024

Hey @kcargile — Just to confirm, is the file being called in the demo via unpkg, the minified version of this refactor?

https://unpkg.com/textblock/textblock.min.js

demo/index.html

@theorosendorf

It points to the minified version of the most recent stable version.

In other words, it won't point to the refactored version until the PR is merged and a new major stable version is published on NPM. That is, demo is pointing to the older stable version. Not ideal since there's technically drift there, but I thought it didn't make sense to pin a version on the demo, since this would mean having to update the unpkg import every time any type of change (patch, minor, major) was merged into the mainline...

There's probably a way to automate this (pinning the unpkg version) via CI/CD so that it's always consistent. If you want to open an issue, I'll explore some options.

@kcargile
Copy link
Collaborator Author

@theorosendorf

All of this is just find and replace, right?

Ya. Find and replace in the code, docs, demo site, and so on.

@theorosendorf
Copy link
Collaborator

Hey @kcargile – I included this on a local version of the textblock.io site and it doesn't seem to be scaling the font sizes, only line heights. I switched to the syntax that the RC1.0.1 version is using. Any thoughts? Would it be helpful for you to see that branch of textblock.io I'm working in?

@kcargile
Copy link
Collaborator Author

Rut roh.

Let me take a look and see if I can figure out what's broken.

@theorosendorf
Copy link
Collaborator

Here's the branch I'm testing in if you need to look there: https://github.com/theorosendorf/textblock.io/tree/textblock-refactor-test

@kcargile
Copy link
Collaborator Author

@theorosendorf Did changing the variable names fix this? I'm using the RC version on GRF and it seems to be working there 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants