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

Add support for multiple locales on Entries #72

Merged
merged 1 commit into from
Nov 2, 2015

Conversation

dlitvakb
Copy link
Contributor

expect(asset.file.properties[:details]['size']).to eq 522943
expect(asset.file.properties[:details][:image]['width']).to eq 5800
expect(asset.file.properties[:details][:image]['height']).to eq 4350
expect(asset.file.properties[:details][:size]).to eq 522943
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a breaking change, can we avoid that?

@dlitvakb dlitvakb force-pushed the dl/allow-multi-locales branch from d815a22 to f529ed5 Compare October 27, 2015 08:55
@dlitvakb
Copy link
Contributor Author

Hey @neonichu I found what was causing the breaking change and fixed it, now everything should work 👍

!self.is_a?(Asset) &&
!is_location?(value) &&
!is_link?(value) &&
!is_image?(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, can we maybe reverse the logic here? Otherwise, I could see us ending up with inconsistencies in which keys are symbols and which are strings very easily if we don't remember amending the list of opt-out cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will be address in a future cleanup of nested locale handling in general, see #73

@dlitvakb
Copy link
Contributor Author

dlitvakb commented Nov 2, 2015

@neonichu can we merge this? We need it for contentful_middleman

@neonichu
Copy link
Contributor

neonichu commented Nov 2, 2015

@dlitvakb Yes, out of GH notifications, out of mind 😞

neonichu added a commit that referenced this pull request Nov 2, 2015
Add support for multiple locales on Entries
@neonichu neonichu merged commit bf5df07 into master Nov 2, 2015
@neonichu neonichu deleted the dl/allow-multi-locales branch November 2, 2015 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants