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

update speakers-company-logo #9

Merged
merged 8 commits into from
Nov 28, 2018
Merged

update speakers-company-logo #9

merged 8 commits into from
Nov 28, 2018

Conversation

rnrnshn
Copy link
Collaborator

@rnrnshn rnrnshn commented Nov 27, 2018 •
edited
Loading

updated the company logo below the speaker's photo to gdgmaputo logo as said in Update speakers info #8

screenshot_12

@rnrnshn rnrnshn requested a review from osvaldoM November 27, 2018 23:19
@osvaldoM
Copy link
Member

@rosariopfernandes can you test if this is working ? I'm unable to test it at the moment.

thatfiredev
thatfiredev requested changes Nov 28, 2018
View reviewed changes
Copy link

@thatfiredev thatfiredev left a comment

Choose a reason for hiding this comment

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

The field companyLogoUrl should contain a full URL and not a path to the image. I don't know the actual reason why, but that's how the template was structured when we got it, so I suggest we keep it that way:

rnrnshn
rnrnshn commented Nov 28, 2018
View reviewed changes
Copy link
Collaborator Author

@rnrnshn rnrnshn left a comment

Choose a reason for hiding this comment

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

Thanks @rosariopfernandes I was looking for this url

@rnrnshn rnrnshn requested a review from thatfiredev November 28, 2018 12:29
thatfiredev and others added 7 commits November 28, 2018 14:30
thatfiredev
thatfiredev approved these changes Nov 28, 2018
View reviewed changes
Copy link

@thatfiredev thatfiredev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! I actually have some changes to add to the list of speakers, so I'll only deploy this once my changes are pushed.

@thatfiredev
Copy link

@0limpi0 on a side note: GitHub now supports batching suggested changes in a single commit (beta). See this tweet

@thatfiredev thatfiredev merged commit 4d311ab into master Nov 28, 2018
@thatfiredev thatfiredev deleted the update-speakers-company-logo branch November 28, 2018 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thatfiredev thatfiredev thatfiredev approved these changes

@osvaldoM osvaldoM Awaiting requested review from osvaldoM

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rnrnshn @osvaldoM @thatfiredev