Skip to content

refactor: migrate events screen to styled-components#687

Merged
housseindjirdeh merged 3 commits intogitpoint:masterfrom
ZahraTee:styled-components-events-screen
Jan 8, 2018
Merged

refactor: migrate events screen to styled-components#687
housseindjirdeh merged 3 commits intogitpoint:masterfrom
ZahraTee:styled-components-events-screen

Conversation

@ZahraTee
Copy link
Copy Markdown
Contributor

@ZahraTee ZahraTee commented Jan 6, 2018

Before After
events-before events-after

I also noticed the branch name (see first event) wasn't getting the LinkBranchDescription code style as it does in 'create events' despite the fact GH also has that styling for 'push events'. So I added that.

(Part of #532)

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 43.192% when pulling 0f28b67 on ZahraTee:styled-components-events-screen into 317f73c on gitpoint:master.

Copy link
Copy Markdown
Member

@chinesedfan chinesedfan left a comment

Choose a reason for hiding this comment

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

@ZahraTee Thanks for your continuous PRs! Left some comments to you.

Comment thread src/auth/screens/events.screen.js Outdated
flex: 1;
margin-left: 10;
color: ${colors.primaryDark};
font-family: ${fonts.fontPrimaryLight.fontFamily};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For styled components fonts, try to import from "src/config/styled-fonts.js".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I used fonts because I remember seeing conflicting info on whether there was a plan to get rid of styledFonts on some PRs because it was duplicating the font listing... like a comment by @machour in #557 and the discussion in #533.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ZahraTee Yes, I see two kinds of codes. How about choosing any one but don't creating the 3rd one? 😄

  • ${fonts.fontPrimaryLight};
  • font-family: ${styledFonts.fontPrimaryLight};

Comment thread src/auth/screens/events.screen.js Outdated
);
case 'ReleaseEvent':
return `${userEvent.payload.release.id}`;
return `888 ${userEvent.payload.release.id}`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

888? Seems like you mistyped something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops. Forgotten remnants of my pro debugging technique... 😅

Comment thread src/auth/screens/events.screen.js Outdated
>
{userEvent.repo.name}
</Text>
999 {userEvent.repo.name}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

999

Comment thread src/auth/screens/events.screen.js Outdated
font-family: ${fonts.fontCode.fontFamily};
`;

const Date = styled.Text`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Date" sounds too general. It usually reminds me "new Date()" in Javascript. How about "DateText" or something else?

<LinkBranchDescription>
{userEvent.payload.ref.replace('refs/heads/', '')}
</Text>
</LinkBranchDescription>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice catch! Then we can update together to resolve #521.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 43.319% when pulling c0940a7 on ZahraTee:styled-components-events-screen into 317f73c on gitpoint:master.

@machour
Copy link
Copy Markdown
Member

machour commented Jan 7, 2018

I'm all for dropping styledFonts and using {...fonts.xxxx}.
A PR switching the remaining uses of styledFonts and getting rid of it would be awesome!

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 43.319% when pulling 00a378b on ZahraTee:styled-components-events-screen into e1357a5 on gitpoint:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 43.319% when pulling 00a378b on ZahraTee:styled-components-events-screen into e1357a5 on gitpoint:master.

Copy link
Copy Markdown
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Thanks again @ZahraTee - you've helped SO much already converting to styled-components and I can't thank you enough 🙏

@housseindjirdeh housseindjirdeh merged commit 3115cef into gitpoint:master Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants