Skip to content

Drop keyframesName function, exposing KeyframesName constructor instead. #34

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

Merged
merged 2 commits into from
May 12, 2023

Conversation

nsaunders
Copy link
Owner

Description

This change drops the keyframesName function and exposes the KeyframesName constructor in its place.

Design considerations

First, a little background: The keyframesName function was intended as a smart constructor "placeholder" of sorts. I got the idea that it might be important when I noticed that a <keyframes-name> value is a <custom-ident> meaning that certain values aren't allowed, such as certain keywords. Although the function's signature keyframesName :: String -> KeyframesName didn't convey the notion of failure (e.g. through a Maybe KeyframesName return type), my high-level plan was to eventually add some logic to make any invalid String input into a valid KeyframesName value by changing the input in some way.

Now I am thinking this would likely involve a complicated implementation, likely an imperfect one, and probably doesn't offer enough benefit to justify that cost.

For comparison, I also had a look at Halogen, whose ClassName type (moved to web-html in 2020) is analogous to KeyframesName. Here's what I found:

  • No smart constructor; the ClassName constructor is exposed instead. See Web.HTML.Common.
  • There used to be a className constructor function, but it was removed many years ago without explanation.
  • The aforementioned className function never included any validation or other logic to ensure that the ClassName return value would be a valid class name (e.g. free of whitespace). Perhaps this is why it was pruned.

Thus, in removing keyframesName I am following suit with the leading PureScript UI framework, whose core maintainers' judgment I trust, even if they didn't document their rationale in this case.

Moreover, I am considering (#29) adopting the ClassName and AttrName types (from web-html, previously halogen). Since their constructors are exposed directly, I think the same pattern should be followed here for consistency's sake.

Future plans

Revisit this issue if changes are made in Web.HTML.Common.

References

Code change checklist

  • Any new or updated functionality includes corresponding unit test coverage.
  • I have verified code formatting, run the unit tests, and checked for any changes in the examples.
  • I have added an entry to the Unreleased section of the CHANGELOG.

@nsaunders nsaunders merged commit 5bc2d2d into master May 12, 2023
@nsaunders nsaunders added enhancement New feature or request breaking Non-backward-compatible API change labels May 13, 2023
@nsaunders nsaunders deleted the feature/drop-fn-keyframes branch July 27, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Non-backward-compatible API change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant