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 avatar lightbox #932

Merged
merged 4 commits into from
Oct 29, 2017
Merged

Add avatar lightbox #932

merged 4 commits into from
Oct 29, 2017

Conversation

Sekhmet
Copy link
Contributor

@Sekhmet Sekhmet commented Oct 29, 2017

Closes #926

Changes:

  • Add AvatarLightbox component that renders avatar and lightbox for that avatar with given image size.

avatar-lightbox

@bonustrack bonustrack temporarily deployed to busy-dev-pr-932 October 29, 2017 10:39 Inactive
@Sekhmet Sekhmet self-assigned this Oct 29, 2017
@bonustrack
Copy link
Contributor

@Sekhmet Isn't better to use https://ant.design/components/carousel/ for it and remove react-image-lightbox in favor of antd for single post too?

@Sekhmet
Copy link
Contributor Author

Sekhmet commented Oct 29, 2017

I don't think this would be good candidate for lightbox, as it is designed as carousel.
Lightbox renders on top of everything by default and supports zoom and pan, which is helpful for big images. If we are going to switch to carousel we would have to implement those features ourselves.

Copy link
Contributor

@jm90m jm90m left a comment

Choose a reason for hiding this comment

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

LGTM, I actually like the way lightbox looks, only problem with it is that there doesn't seem to be too much community support around it. But it looks good in mobile responsive.. I guess @bonustrack can decide :)

};

static defaultProps = {
username: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to use the default avatar username here? I guess it doesn't matter if it fails on that request then it goes to avatar default image anyways. So your call :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@steemconnect uses different avatar than we use as error fallback. I don't know whether there is username that has avatar set to that default avatar.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong username got default avatar example https://img.busy.org/@xxxx123456 . We should not rely on @steemconnect avatar, at some point we will want to have the logo of SC as avatar.

@bonustrack bonustrack temporarily deployed to busy-dev-pr-932 October 29, 2017 13:36 Inactive
@Sekhmet
Copy link
Contributor Author

Sekhmet commented Oct 29, 2017

I've updated Avatar so it uses defaultImage instead of steemconnect as username if username is undefined.

@bonustrack
Copy link
Contributor

@Sekhmet The image size should not be bigger than the original and also should keep aspect ratio. You can do this by changing the crop style like this:
http://img.busy.org/@blocktrades?crop=limit&s=800
http://img.busy.org/@fabien?crop=limit&s=800

@Sekhmet Sekhmet merged commit ef0b4c2 into busyorg:new-design Oct 29, 2017
@Sekhmet Sekhmet deleted the avatar-lightbox branch October 29, 2017 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants