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 get profile posts use-case with Facebook map #6

Merged
merged 4 commits into from Jan 17, 2022

Conversation

janhalama
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@jnv jnv left a comment

Choose a reason for hiding this comment

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

👍 Great work here, just a few minor issues.

src/app.js Show resolved Hide resolved
src/app.js Outdated
});
});

app.post('/profile-posts', ensureAuthenticated, require('./profile-posts'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of displaying it under POST submission, I suggest adding it as a query parameter to GET /profile-posts, e.g. /profile-posts?provider=facebook&profileId=1234

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

src/sf/grid/social-media/posts.facebook.suma Show resolved Hide resolved
return {
type = attachment.type
url = attachment.url
altText = attachment.title
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently it's not possible to get an alt text from FB API (or it's undocumented). Personally I'd rather extend the profile to include title and description (probably as text) from the attachment.

Copy link
Collaborator Author

@janhalama janhalama Jan 17, 2022

Choose a reason for hiding this comment

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

I added title and description as you suggested.

Copy link
Collaborator

@jnv jnv left a comment

Choose a reason for hiding this comment

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

LGTM

@janhalama janhalama changed the title WIP: Add get profile posts use-case with Facebook map Add get profile posts use-case with Facebook map Jan 17, 2022
@janhalama janhalama merged commit 00c507e into main Jan 17, 2022
@jnv jnv deleted the feat/add_posts_profile branch January 17, 2022 13:35
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.

None yet

2 participants