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.

Sign up for GitHub

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

Jump to bottom

[RFC][UN-16593] Add support to all currencies #305

Merged
merged 2 commits into from
Jan 23, 2019
Merged

[RFC][UN-16593] Add support to all currencies #305

merged 2 commits into from
Jan 23, 2019

Conversation

rntdrts
Copy link
Contributor

@rntdrts rntdrts commented Jan 14, 2019

AndreJoaquim
AndreJoaquim reviewed Jan 15, 2019
View reviewed changes
addon/enums/currency-symbol-type.js Outdated
@@ -1,8 +0,0 @@
import Enumerable from './enumerable';
Copy link
Contributor

Choose a reason for hiding this comment

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

My two cents would be not erasing this one: this is used in practically all of our projects, and I don't think we have the time to migrate them just now...

Copy link
Contributor Author

@rntdrts rntdrts Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Well if we have the time to upgrade all of our projects to this version we should have the time to upgrade this... it's minor compared to the upgrade of ember cli uniq...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes... But I don't know if we want to do that right now. And this will block all future versions of this add-on. I would prefer keeping them both, talk about it in the sync, and start migrating when we have time.

Copy link
Contributor

Choose a reason for hiding this comment

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

But, in the end, it's not my decision cc @TiagoBrito

@tlfbrito
Copy link
Contributor

tlfbrito commented Jan 22, 2019 •
edited
Loading

We should minimize the BC's (breaking changes) as much as possible.
Please don't remove addon/enums/currency-symbol-type.js and mark it as deprecated.

/**
 * @deprecated since version xxx. Please use zzzz instead.
 */

@tlfbrito tlfbrito merged commit c31169c into master Jan 23, 2019
@tlfbrito tlfbrito deleted the UN-16593/add-support-to-all-currencies branch January 23, 2019 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreJoaquim AndreJoaquim AndreJoaquim left review comments

@tlfbrito tlfbrito tlfbrito approved these changes

@mmelo mmelo mmelo approved these changes

@sardyy sardyy Awaiting requested review from sardyy

@DaniFrancisco DaniFrancisco Awaiting requested review from DaniFrancisco

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rntdrts @tlfbrito @mmelo @AndreJoaquim