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

[Mobile v1] Overlay thread participant heads #3072

Conversation

TheMightyPenguin
Copy link
Contributor

@TheMightyPenguin TheMightyPenguin commented May 11, 2018

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • mobile

Release notes for users (delete if codebase-only change)
Tired of seeing those heads so spaced out? Well no more, now they stack in a sexy way!

Before:
participants_before

After:
participants_after

Parent Isue: #2330

@spectrum-bot
Copy link

spectrum-bot bot commented May 11, 2018

Warnings
⚠️

These modified files do not have Flow enabled:

  • mobile/components/Avatar/style.js

Generated by 🚫 dangerJS

@TheMightyPenguin TheMightyPenguin force-pushed the overlay-mobile-thread-participant-heads branch 2 times, most recently from 439b426 to 773f016 Compare May 12, 2018 01:23
@TheMightyPenguin TheMightyPenguin force-pushed the overlay-mobile-thread-participant-heads branch from 773f016 to e985401 Compare May 12, 2018 04:09
Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Thanks so much for digging into this—overall it looks great, just a tiny code nitpick!

`;

export const StackedEmptyParticipantHead = stackizeHeads(EmptyParticipantHead);
export const StackedAvatar = stackizeHeads(Avatar);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the css helper here instead of wrapping it in another component:

import { css } from 'styled-components';

const stackingStyles = css`
  margin-right: -10px;
  border-width: 2px;
  border-color: #FFFFFF;
`

export const StackedEmptyParticipantHead = styled(EmptyParticipantHead)`
  ${stackingStyles}
`

export const StackedAvatar = styled(Avatar)`
  ${stackingStyles}
`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make this change 👍. Should I keep the HoC, or is it overkill for this? @mxstbr

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the HoC

@brianlovin
Copy link
Contributor

Looking awesome @TheMightyPenguin!

@@ -53,11 +54,24 @@ export const FacepileContainer = styled.View`
export const EmptyParticipantHead = styled.Text`
color: ${props => props.theme.text.alt};
background: ${props => props.theme.bg.wash};
border-radius: 15px;
border-radius: ${props => (props.radius ? `${props.radius}px` : '15px')};
Copy link
Contributor

Choose a reason for hiding this comment

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

We should handle all this styling at the mobile/components/Avatar level. border-radius for user avatars should always be 100%. for community avatars, it should always be 25%.

The only thing that should change between instances is size and whether or not it has status and link elements.

It's ok to handle the overlapping here, of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

I learned tonight that react native does some weird things with border radius; for circles you have to set the border radius to 50% of the width/height, otherwise it over-radius-es and becomes a diamond :P

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh also just note this is an empty participant head - not an avatar, so we'll need to treat it differently anyways :)

@TheMightyPenguin
Copy link
Contributor Author

@mxstbr PR updated 😄

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you so much @TheMightyPenguin!

@mxstbr mxstbr merged commit 26fc574 into withspectrum:alpha May 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants