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

avatar cleanup #18824

Merged
merged 4 commits into from Aug 7, 2019

Conversation

@chrisnojima
Copy link
Contributor

commented Aug 7, 2019

Cleanup the avatar rendering a little bit post the url changes

chrisnojima added some commits Aug 7, 2019

WIP
WIP
WIP
WIP

@chrisnojima chrisnojima changed the title Nojima/hotpot 502 avatar cleanup avatar cleanup Aug 7, 2019

@@ -88,7 +88,7 @@ class ExplodingPopupHeader extends React.Component<PropsWithTimer<Props>, State>
<Box2 direction="horizontal">
<Text type="BodySmall">by</Text>
<Box2 direction="horizontal" gap="xtiny" gapStart={true} style={styles.user}>
<Avatar username={author} size={16} clickToProfile="tracker" />

This comment has been minimized.

Copy link
@chrisnojima

chrisnojima Aug 7, 2019

Author Contributor

don't ever spawn the tracker

const name = isTeam ? ownProps.teamname : ownProps.username
const urlMap = avatarSizes.reduce((m, size: number) => {
const onClick =
dispatchProps.onClick || (username ? () => dispatchProps._goToProfile(username) : undefined)

This comment has been minimized.

Copy link
@chrisnojima

chrisnojima Aug 7, 2019

Author Contributor

this makes more things clickable, which seems fine to me (cc: @cecileboucheron )

@chrisnojima chrisnojima marked this pull request as ready for review Aug 7, 2019

@chrisnojima

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@jakob223
Copy link
Contributor

left a comment

Woo so much cleaner now

LGTM as long as Cecile is ok with more avatars being links

: dispatch(Tracker2Gen.createShowUser({asTracker: true, username})),
}),
(dispatch, ownProps: OwnProps) => ({
_goToProfile: (username: string) => dispatch(ProfileGen.createShowUserProfile({username})),
onClick: ownProps.onEditAvatarClick ? ownProps.onEditAvatarClick : ownProps.onClick,

This comment has been minimized.

Copy link
@jakob223

jakob223 Aug 7, 2019

Contributor

why even do this in dispatch props?

This comment has been minimized.

Copy link
@chrisnojima

chrisnojima Aug 7, 2019

Author Contributor

mergeProps evaluates username, so it should delegate it. also we likely want to avoid ownProps in mapState and mapDispatch if possible

@@ -150,7 +150,7 @@ export const collapseStyles = (styles: ReadonlyArray<T.CollapsibleStyle>): Objec
(a: Array<T.CollapsibleStyle>, e: T.CollapsibleStyle) => a.concat(e),
[]
) as Array<Object | null | false>
const style = flattenedStyles.reduce<Object>((o, e) => (e ? {...o, ...e} : o), {})
const style = flattenedStyles.reduce<Object>((o, e) => Object.assign(o, e), {})

This comment has been minimized.

Copy link
@jakob223

jakob223 Aug 7, 2019

Contributor

Isn't (o, e) => Object.assign(o, e) just Object.assign

This comment has been minimized.

Copy link
@chrisnojima

chrisnojima Aug 7, 2019

Author Contributor

its very dangerous to pass along functions from one place to another w/o constraint. for example here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Syntax
Object.assign takes arbitrary params and merges them, so you can do
Object.assign(a,b,d,e,f) etc
what does reduce pass (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce#Syntax)
accumulator, currentValue[, index[, array]])[, initialValue], meaning if we just passed Object.assign to reduce it'd pass alllllllll those params in, which isn't what we want.

This pattern happens elsewhere. It's better to just be explicit about it

@mmaxim

mmaxim approved these changes Aug 7, 2019

@chrisnojima chrisnojima merged commit c167df7 into master Aug 7, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@chrisnojima chrisnojima deleted the nojima/HOTPOT-502-avatar-cleanup branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.