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

Visual tweaks #5672

Merged
merged 10 commits into from
Feb 7, 2017
Merged

Visual tweaks #5672

merged 10 commits into from
Feb 7, 2017

Conversation

cecileboucheron
Copy link
Contributor

No description provided.

@cecileboucheron
Copy link
Contributor Author

@keybase/react-hackers

@chromakode chromakode self-requested a review February 6, 2017 19:06
Copy link
Contributor

@chromakode chromakode left a comment

Choose a reason for hiding this comment

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

LGTM if tests pass!

@@ -9,7 +9,7 @@ import type {Props} from './header'
const Header = ({participants, onOpenFolder, onToggleSidePanel, sidePanelOpen, you, metaDataMap, followingMap, onShowProfile}: Props) => (
<Box style={containerStyle}>
<Usernames colorFollowing={true} inline={false} commaColor={globalColors.black_40} type='BodyBig' users={usernamesToUserListItem(participantFilter(participants, you).toArray(), you, metaDataMap, followingMap)}
containerStyle={{flex: 1, textAlign: 'center', justifyContent: 'center'}} onUsernameClicked={onShowProfile} />
containerStyle={{flex: 1, textAlign: 'center', justifyContent: 'center', marginLeft: 48}} onUsernameClicked={onShowProfile} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Would globalMargins.large be appropriate here instead of 48?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, no globalMargins.large is 40px, not 48. This tweak is to recenter the usernames in the header by pushing them by 2 times 24 (from both icons on the right side).

@@ -139,7 +139,7 @@ class Avatar extends Component<void, Props, State> {
{this.props.backgroundColor &&
<div
style={{...avatarStyle,
...borderStyle,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this removes the logic that handles a this.props.borderColor passed in to the component. I don't see it being used anywhere, so you can probably delete the const borderStyle = this.props.borderColor ? {borderRadius... line too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Max and I chatted live about this - the borderColor attribute is used in the nav bar so we're keeping it.

{badgeNumber > 0 &&
<Box style={{...styleBadgeNav}}>
<Badge badgeNumber={badgeNumber} badgeStyle={{marginLeft: 0, marginRight: 0}} />
<Badge badgeNumber={badgeNumber} badgeStyle={{marginLeft: 0, marginRight: 8}} outlineColor={globalColors.midnightBlue} />
Copy link
Contributor

Choose a reason for hiding this comment

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

globalMargins.tiny?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@keybase-ci-visdiff
Copy link

The commits 92afd2a...045ebed introduce visual changes on linux.

🔎 7 new, 19 changed

@keybase-ci-visdiff
Copy link

The commits 92afd2a...045ebed introduce visual changes on linux.

🔎 7 new, 19 changed

@keybase-ci-visdiff
Copy link

The commits 1f9d0f5...bc853b7 introduce visual changes on linux.

🔎 7 new, 19 changed

@cecileboucheron cecileboucheron merged commit ce0f90d into master Feb 7, 2017
@cecileboucheron cecileboucheron deleted the cecile/DESKTOP-cecile-visual-tweaks branch February 7, 2017 00:41
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