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

Use number field for decimal preference field #2232

Closed
wants to merge 1 commit into from
Closed

Use number field for decimal preference field #2232

wants to merge 1 commit into from

Conversation

swcraig
Copy link
Contributor

@swcraig swcraig commented Sep 20, 2017

There are two benefits to this. The UX benefit of the
increment/decrement widget on the input field itself, and now a user can
no longer enter text into this field (which is rejected upon submit and
the preference value can be reset to 0).

Adding step: :any allows both integers and floats to be entered, with the widget stepping up and down by 1.

I know that the preference_field_tag is deprecated, but this threw me off when I was testing something related.

There are two benefits to this. The UX benefit of the
increment/decrement widget on the input field itself, and now a user can
no longer enter text into this field (which is rejected upon submit and
the preference value is reset to 0).
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

👍

Only the helper is deprecated the field partials are not. Actually I introduced them recently while deprecating the helper.

kennyadsl
kennyadsl previously approved these changes Sep 21, 2017
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

One thing I just encountered is that browsers format the decimals depending from the locale of the system (at least in macOS). So, Safari and Chrome display decimals with a comma separator (as my locale is Germany) and for US locale it displays a period separator. And this is independent from the website locale (en in the case of the solidus sandbox). It only respects the OS setting.

This is not a huge deal, because most of the time this is what users will expect. But it is strange to see all numbers localized as English numbers in the rest of the ui, but in the input field it is localized differently.

OS set to US locale

new payment - payments - r929121340 - orders 2017-09-21 14-15-33

OS set to German locale

new payment - payments - r929121340 - orders 2017-09-21 14-15-57

Ideally browser vendors would respect the locale setting of the website, but this should not block this PR.

@kennyadsl
Copy link
Member

@tvdeyen good catch! I also found another PR (#1821) that adds other thoughts to this topic. I'm going to remove my approval since I feel like I need to revisit this changes better.

@kennyadsl kennyadsl dismissed their stale review September 22, 2017 21:31

Need to revisit taking into account #1821

@jhawthorn
Copy link
Contributor

My feeling from working on #1821 is that number inputs are terrible for decimal inputs. Having the input step by 1 isn't desirable for, say, entering a tax rate. The bad browser behaviour w/r/t locale only makes this all worse.

I'd rather continue using input type=text, even if it's semantically less correct.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I remove my approval, because of the quirks I mentioned above and John in #1821

@mamhoff
Copy link
Contributor

mamhoff commented Oct 4, 2017

Closing due to localization issues with browser vendors.

@mamhoff mamhoff closed this Oct 4, 2017
@swcraig swcraig deleted the decimal-preference-field-to-number-field branch October 4, 2017 16:21
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.

5 participants