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

Fix number formatting #408

Merged
merged 7 commits into from
Jan 29, 2025

Conversation

MrSkwiggs
Copy link
Contributor

I wonder if there are many other places where we use the user's localized number formatting instead of a static en-US format. Will double check the codebase and report back

@MrSkwiggs
Copy link
Contributor Author

Only places I could find which used the locale-dependant call to .format() where in Slide.swift, LineSpacing.swift & Opacity.swift

@MrSkwiggs
Copy link
Contributor Author

For now I've implemented static number formatting, but perhaps this could be included in the site configuration and passed along where necessary

@JPToroDev
Copy link
Collaborator

JPToroDev commented Jan 28, 2025

Thank you for catching this!

Instead of requiring the use of NumberFormatter, could we create a computed property like oneDecimalPlace in extensions for Percentage, Double, and Float?

@MrSkwiggs MrSkwiggs force-pushed the bugfix/opacity-number-formatting branch from 8a37ee6 to 97e5594 Compare January 29, 2025 08:42
@MrSkwiggs
Copy link
Contributor Author

Instead of requiring the use of NumberFormatter, could we create a computed property like oneDecimalPlace in extensions for Percentage, Double, and Float?

I've implemented it as an extension on FormatStyle directly, much cleaner this way. Also took the liberty of using up to 3 decimal places for opacity, but kept the rest single-decimal precision. Could make the default 3 decimal places always?

@JPToroDev
Copy link
Collaborator

I've implemented it as an extension on FormatStyle directly, much cleaner this way.

Awesome!

Could we make the extension a little more concise? nonLocalizedDecimal(_ decimalPlaces: Int = 1) ?

Could make the default 3 decimal places always?

I like the way you have it 🙂

@MrSkwiggs
Copy link
Contributor Author

Could we make the extension a little more concise? nonLocalizedDecimal(_ decimalPlaces: Int = 1) ?

value.formatted(.nonLocalizedDecimal(3)) isn't a super obvious callsite in my opinion :/
Maybe a good compromise would be value.formatted(.nonLocalizedDecimal(precision: 3)) makes it a bit more concise while still being clear?

Let me know 🙏

@MrSkwiggs
Copy link
Contributor Author

Also I see that we might be having a race condition since the changes to the PublishingContext were made, as last tests failed with Ignite/PublishingContext.swift:22: Fatal error: "PublishingContext.default accessed before being initialized. Call PublishingContext.initialize() first. Running tests locally a few times I could reproduce this as well. Will make an issue for it 👌

@JPToroDev
Copy link
Collaborator

How about length or places then? Would shave off a few more character

@JPToroDev
Copy link
Collaborator

Also I see that we might be having a race condition since the changes to the PublishingContext were made, as last tests failed with

Yes, the way we're currently approaching tests doesn't play nice with PublishingContext, but #420 sorts this all out 🙌🏼

@MrSkwiggs MrSkwiggs force-pushed the bugfix/opacity-number-formatting branch from b5222f5 to 157829f Compare January 29, 2025 14:40
@MrSkwiggs
Copy link
Contributor Author

How about length or places then? Would shave off a few more character

Done, using places 👍

@JPToroDev JPToroDev merged commit e1d71f4 into twostraws:main Jan 29, 2025
1 check failed
@JPToroDev
Copy link
Collaborator

Excellent—thank you!

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