Skip to content
This repository has been archived by the owner on Nov 28, 2022. It is now read-only.

🔥✨ Adapt image property changes #661

Merged
merged 2 commits into from
Apr 24, 2017
Merged

🔥✨ Adapt image property changes #661

merged 2 commits into from
Apr 24, 2017

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Apr 20, 2017 •
edited
Loading

refs TryGhost/Ghost#8348

  • test signup
  • test all image cases again

@kirrg001 kirrg001 mentioned this pull request Apr 20, 2017
Merged
9 tasks
@kirrg001 kirrg001 mentioned this pull request Apr 20, 2017
Merged
@kirrg001 kirrg001 force-pushed the fix/image-properties branch 6 times, most recently from 9c14ec7 to 10cff49 Compare April 24, 2017 09:58
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 73.336% when pulling 10cff49 on kirrg001:fix/image-properties into 0d385df on TryGhost:master.

@kirrg001 kirrg001 changed the title [WIP] 🔥✨ Adapt image property changes 🔥✨ Adapt image property changes Apr 24, 2017
@kirrg001 kirrg001 requested a review from aileen April 24, 2017 11:21
@kevinansfield
Copy link
Member

@kirrg001 there shouldn't be any underscored names in the client, all properties/attributes/methods are camelCase so for example feature_image should be featureImage

@kirrg001
Copy link
Contributor Author

kirrg001 commented Apr 24, 2017 •
edited
Loading

@kevinansfield 👍
see here why

@kirrg001 kirrg001 force-pushed the fix/image-properties branch 2 times, most recently from c34f638 to 689703a Compare April 24, 2017 12:27
@kirrg001 kirrg001 changed the title 🔥✨ Adapt image property changes [WIP] 🔥✨ Adapt image property changes Apr 24, 2017
@kirrg001
Copy link
Contributor Author

WIP again, testing something with image deletion.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 689703a on kirrg001:fix/image-properties into ** on TryGhost:master**.

@kirrg001 kirrg001 changed the title [WIP] 🔥✨ Adapt image property changes 🔥✨ Adapt image property changes Apr 24, 2017
@kirrg001
Copy link
Contributor Author

kirrg001 commented Apr 24, 2017 •
edited
Loading

Ok this is ready to review now. Please pull latest changes.

@kirrg001
🙀 adapt image property changes
0d0032a 
refs TryGhost/Ghost#8348

- rename all image properties
- e.g. author.image --> author.profile_image
- test all image functionality
aileen
aileen approved these changes Apr 24, 2017
View reviewed changes
Copy link
Member

@aileen aileen left a comment

Choose a reason for hiding this comment

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

Tested and LVGTM 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 73.376% when pulling 0d0032a on kirrg001:fix/image-properties into 1f3b79e on TryGhost:master.

kevinansfield
kevinansfield reviewed Apr 24, 2017
View reviewed changes
app/models/setting.js
@@ -9,7 +9,7 @@ export default Model.extend(ValidationEngine, {
title: attr('string'),
description: attr('string'),
logo: attr('string'),
cover: attr('string'),
cover_image: attr('string'),

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

kirrg001 added a commit to kirrg001/Ghost-Admin that referenced this pull request Apr 24, 2017
@kirrg001
🎨 settings inconsistency
c19f4c7 
no issue

- discussed here TryGhost#661 (comment)
kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Apr 24, 2017
@kirrg001
🎨 settings inconsistency
c39c6ef 
no issue

- discussed here TryGhost/Admin#661 (comment)
This was referenced Apr 24, 2017
Merged
Merged
kevinansfield
kevinansfield suggested changes Apr 24, 2017
View reviewed changes
Copy link
Member

@kevinansfield kevinansfield left a comment

Choose a reason for hiding this comment

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

I think one place has been missed - post.author.image here needs changing to post.author.profileImage https://github.com/TryGhost/Ghost-Admin/blob/master/app/components/gh-posts-list-item.js#L31-L32

Otherwise LGTM 👍

@kirrg001
Fix post.author.image!
d588f21 
- this one slipped through because there is no UI usage at the moment
@kirrg001
Copy link
Contributor Author

@kevinansfield Thanks!

I have pushed it as separate commit d588f21

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 73.376% when pulling d588f21 on kirrg001:fix/image-properties into 1f3b79e on TryGhost:master.

@kevinansfield kevinansfield merged commit 7169a14 into TryGhost:master Apr 24, 2017
kirrg001 added a commit to kirrg001/Ghost-Admin that referenced this pull request Apr 24, 2017
@kirrg001
🎨 settings inconsistency
eac3f39 
no issue

- discussed here TryGhost#661 (comment)
kevinansfield pushed a commit to TryGhost/Ghost that referenced this pull request Apr 24, 2017
@kirrg001 @kevinansfield
🎨 settings inconsistency (#8381)
4e2474a 
no issue
- replace camelCase settings keys with underscore_case for consistency
- discussed here TryGhost/Admin#661 (comment)
kevinansfield pushed a commit that referenced this pull request Apr 24, 2017
@kirrg001 @kevinansfield
🎨 settings model inconsistency (#669)
3768905 
requires TryGhost/Ghost#8381
- all camelCase settings model attribute names received from the API are now underscore_case
- discussed here #661 (comment)
@CaiJimmy CaiJimmy mentioned this pull request Apr 25, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kevinansfield kevinansfield kevinansfield approved these changes

@aileen aileen aileen approved these changes

Assignees

@kevinansfield kevinansfield

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kirrg001 @coveralls @kevinansfield @aileen