Fix money deprecation warning. #2912
Conversation
|
Does this cause the UI to break horrendously (see #2870) or is it working? |
|
@jacobherrington I don’t see the html safe issues from the other PR, but there are style breaks that will need to be fixed. Doesn’t look major at all just a line break was added between the currency symbol and value, but I still need to fix specs too. |
|
@jacobherrington I believe specs should be passing now if you want to take another look. I didn't end up having to make any styling changes I just had to remove the gsub changing whitespace into html entity for whitespace as that was causing invalid html to render making the line break. The spec changes are mainly just taking into account the spans that are now rendered, and were not rendered before. In theory this should be fine for most stores, but there is always the possibility some custom styles my conflict... |
core/lib/spree/money.rb
Outdated
| @@ -4,6 +4,8 @@ | |||
| require 'monetize' | |||
| require 'active_support/core_ext/string/output_safety' | |||
|
|
|||
| Money.locale_backend = :i18n | |||
There was a problem hiding this comment.
I think we should not add global configuration in a class like this.
Could we move this into it's own config/initializers/money.rb of the host app instead? And tell users to do so in their current apps and add it into the install generator? I like to be explicit with these things, especially if it's not our gem we are configuring here.
There was a problem hiding this comment.
I agree this should be done in a more configurable manner, but I don’t think we should require everyone to update their apps with an initializer by default. I’m thinking just allow apps to override the setting if they really want to. WDYT?
There was a problem hiding this comment.
I’m not a fan of adding a setting in our app that’s a proxy to a foreign gem.
What about putting it inside a before_initialize block? Are engine config/initialize files read before host apps files? That would be preferable. Someway where the host app is able to set it in a default (read: mentioned in money gems readme) kind of way and we still act as fallback/default?
JDutil
force-pushed
the
hotfix/2870-fix-money-deprecations
branch
from
9c64c98
to
375694b
Compare
core/config/initializers/money.rb
Outdated
| @@ -0,0 +1,5 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
There was a problem hiding this comment.
Layout/TrailingWhitespace: Trailing whitespace detected.
core/config/initializers/money.rb
Outdated
| @@ -0,0 +1,5 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
Layout/CommentIndentation: Incorrect indentation detected (column 1 instead of 0).
|
@tvdeyen I updated this PR so that Money is now initialized in |
JDutil
force-pushed
the
hotfix/2870-fix-money-deprecations
branch
from
75057f5
to
faa3653
Compare
|
@ericsaupe squashed 👍 |
|
This could be a breaking change for some store right? Maybe we should add a specific line into the CHANGELOG. @JDutil what do you think? |
* [DEPRECATION] `use_i18n` is deprecated - use `Money.locale_backend = :i18n` instead * [DEPRECATION] `html` is deprecated - use `html_wrap` instead. Please note that `html_wrap` will wrap all parts of currency and if you use `with_currency` option, currency element class changes from `currency` to `money-currency`. * [DEPRECATION] `symbol_position:` option is deprecated - use `format` to specify the formatting template. * Initialize Money in initializer for main app to override.
JDutil
force-pushed
the
hotfix/2870-fix-money-deprecations
branch
from
e36615c
to
e529d89
Compare
|
@kennyadsl added to the changelog. |
|
Made a test locally and I can't see any UI change from what we previously have, thanks @JDutil 👍 🎉 |
|
@kennyadsl yea there shouldn't be visual changes unless you've customized how your price is displayed for something like having a big number and small decimals or something along those lines or are applying css to a span tag instead of a class. I don't think anyone should really be effected unless they went out of their way to really change the pricing display, but going forward these new spans rendered will allow people to more easily do any custom styles for big currency symbol or number and small decimal etc... |
fixes #2870