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

Enable USE_THOUSANDS_SEPARATOR by default & have some larger test ids #458

Closed
lb- opened this issue Oct 26, 2023 · 5 comments
Closed

Enable USE_THOUSANDS_SEPARATOR by default & have some larger test ids #458

lb- opened this issue Oct 26, 2023 · 5 comments

Comments

@lb-
Copy link
Member

lb- commented Oct 26, 2023

I think it would be good for us to have a way to easily test for common issues that relate to large ID values mixed with the usage of thousands separator.

Problem

<input value={{ my_object.id }} type="checkbox" />

This code will mostly work but if the id is over 1000, it will be rendered as 1,000.

This kind of bug has happened a few times and it would be good for us to more easily see this problem in Bakerydemo.

It's more pronounced depending on language and the setting USE_THOUSAND_SEPARATOR is True.

https://docs.djangoproject.com/en/4.2/ref/settings/#std-setting-USE_THOUSAND_SEPARATOR

Proposal

  • We update the initial data to have a random set of pages, images and maybe even a user or two to have id 1001, 1002 etc.
  • We enable the USE_THOUSAND_SEPARATOR setting by default

Related

@lb- lb- added the demo data label Oct 26, 2023
@gasman
Copy link
Contributor

gasman commented Oct 26, 2023

My one reservation is that I'd rather not encourage greater use of USE_THOUSAND_SEPARATOR in the wild - IMO it really is a terrible design choice to apply formatting by default and require a template filter to turn it off, rather than the other way round - and if we put it in bakerydemo, people will copy it however much we advise against it. Maybe if we put a big capitalised PLEASE DON'T DO THIS comment next to it... :-)

@lb-
Copy link
Member Author

lb- commented Oct 26, 2023

Hmm. That's a good point. Do we know if this kind of issue will appear without that setting on?

Maybe a different approach is to run our Wagtail tests differently, instead of Bakerydemo data we update the data used in unit tests to be numbers above 1000 and run one version of the CI with this setting on?

@RashiJyotishi
Copy link

@lb-
Hi there! I understand the concern with the comma appearing before 3 digits due to the USE_THOUSAND_SEPARATOR setting. If you're looking for assistance in resolving this issue, I'm a beginner but eager to learn, and I'd be happy to give it a shot. Let me know if you'd like me to explore alternative solutions or make adjustments to the code. I'm here to help and keen on gaining more experience!

@gasman
Copy link
Contributor

gasman commented Aug 29, 2024

This shouldn't be necessary any more, now that wagtail/wagtail#12251 is merged.

@gasman gasman closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2024
@lb-
Copy link
Member Author

lb- commented Aug 29, 2024

Awesome. Great work on that PR Matt.

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

No branches or pull requests

3 participants