Skip to content

Pull Request Detail GitHub Enterprise Avatars #1125

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

Merged
merged 5 commits into from
Apr 19, 2019

Conversation

StanleyGoldman
Copy link
Contributor

@StanleyGoldman StanleyGoldman commented Apr 18, 2019

Related to #416

This pull request merged the work from #1121 into #1096 and builds upon it
This corrects the display of user avatars on GitHub Enterprise in a Pull Request Detail

Depends on

@StanleyGoldman StanleyGoldman changed the title Fixing the display of avatars in the new react view Substitute GitHub Enterprise User Avatars in Pull Request Detail Apr 18, 2019
@StanleyGoldman
Copy link
Contributor Author

image

@StanleyGoldman StanleyGoldman changed the title Substitute GitHub Enterprise User Avatars in Pull Request Detail Pull Request Detail GitHub Enterprise Avatars Apr 18, 2019
margin-right: 0;
}

body img.avatar {
body img.avatar,
body span.avatar-icon svg {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we decide to use a conditional filter: invert(100%); rule here, to get the default avatar icon to align with the theme?

@queerviolet
Copy link
Contributor

I can make that change, actually. Thanks for DI'ing the change :)

@queerviolet queerviolet self-requested a review April 19, 2019 20:09
@queerviolet queerviolet merged this pull request into qv/react Apr 19, 2019
this._repositoryReturnsAvatar = true;
}

this._repositoryReturnsAvatar = false;
Copy link
Contributor

@RMacfarlane RMacfarlane May 3, 2019

Choose a reason for hiding this comment

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

@StanleyGoldman I think we're missing an else here, or a return in the 200 check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll send a PR to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

3 participants