Skip to content

refactor: repository-list-item to styled components#605

Merged
machour merged 3 commits intogitpoint:masterfrom
apoeco:master
Jan 16, 2018
Merged

refactor: repository-list-item to styled components#605
machour merged 3 commits intogitpoint:masterfrom
apoeco:master

Conversation

@apoeco
Copy link
Copy Markdown
Contributor

@apoeco apoeco commented Oct 29, 2017

Related #532

Screenshots

Before After
before after

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.4%) to 39.745% when pulling 591bc7d on apoeco:master into 2fda53e on gitpoint:master.

@andrewda
Copy link
Copy Markdown
Member

Could you add before/after screenshots, as provided in the PR template?

@apoeco apoeco changed the title refactor(repository-list-item): Refactor RepositoryListItem refactor: repository-list-item to styled components Oct 29, 2017
@machour machour self-assigned this Nov 4, 2017
Copy link
Copy Markdown
Member

@machour machour left a comment

Choose a reason for hiding this comment

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

Hi @apoeco and thank you for this PR!

I left you a comment so that we can get rid of the last standing old style.

Don't hesitate if you need any help on this

fontFamily: styledFonts.fontPrimarySemiBold,
},
});

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.

We should be able to get rid of this last one when #619 gets merged.

The <ListItem> can be then styled as follow:

const Repository = styled(ListItem).attrs({
  titleStyle: {
    color: colors.primaryDark,
    fontFamily: styledFonts.fontPrimarySemiBold,
  },
})``;

the underlayColor prop can also be merged there as it's is a pure presentation prop

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.

#619 is merged, you should now be able to get rid of this last thingy 🎉

@apoeco apoeco force-pushed the master branch 2 times, most recently from c2a8caf to 42091d2 Compare November 9, 2017 13:19
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.4%) to 40.241% when pulling 42091d2 on apoeco:master into 05c386e on gitpoint:master.

Refactor RepositoryListItem using styled-components
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.4%) to 40.241% when pulling 3d4f76c on apoeco:master into 05c386e on gitpoint:master.

@apoeco
Copy link
Copy Markdown
Contributor Author

apoeco commented Nov 9, 2017

I think everything is ready now 😃

@machour
Copy link
Copy Markdown
Member

machour commented Jan 15, 2018

Hi @apoeco and sorry for not getting back to you sooner 😞

Just had time to review this, and it's working flawlessly 🎉

I wanted to fix the conflicts for you, but it seems that I'm unable to do it because you didn't work on a branch of your fork, but rather on master directly. Could you take care of fixing them ? (You just need to retrieve the last .all-contributorsrc version and add yourself again, both .md files will be regenerated)

We'll get this merged as soon as it's done, thank you again!

Copy link
Copy Markdown
Member

@machour machour left a comment

Choose a reason for hiding this comment

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

It would appear that my merge worked after a few hours. Weird.

Just noticed that you were using styledFonts that was removed not so long ago. Left you review comments with the exact changes to be made. Could you take care of this last thing please? 🙏

I'd happily merge this as is and do the changes for you if you're too busy, no worries!

`;

const TitleText = ColoredText.extend`
font-family: ${styledFonts.fontPrimarySemiBold};
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.

Should be:

const TitleText = ColoredText.extend`
  ${fonts.fontPrimarySemiBold};
`;

`;

const DescriptionText = ColoredText.extend`
font-family: ${styledFonts.fontPrimaryLight};
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.

Should be:

const DescriptionText = ColoredText.extend`
  ${fonts.fontPrimaryLight};
`;

padding-left: 3;
padding-top: 2;
margin-right: 15;
font-family: ${styledFonts.fontPrimary};
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.

${fonts.fontPrimary};

const Repository = styled(ListItem).attrs({
titleStyle: {
color: colors.primaryDark,
fontFamily: styledFonts.fontPrimarySemiBold,
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.

fontFamily: fonts.fontPrimarySemiBold.fontFamily,


import { emojifyText, abbreviateNumber } from 'utils';
import { colors, languageColors, fonts, normalize } from 'config';
import { colors, languageColors, styledFonts, normalize } from 'config';
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.

use fonts instead of styledFonts

@apoeco
Copy link
Copy Markdown
Contributor Author

apoeco commented Jan 16, 2018

I’m on it!

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.7%) to 44.005% when pulling b9170cb on apoeco:master into 608818e on gitpoint:master.

padding-top: 2;
margin-right: 15;
font-family: ${styledFonts.fontPrimary};
font-family: ${fonts.fontPrimary};
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.

Almost :D
Remove font-family: and we're good to go

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.4%) to 44.005% when pulling 8e10069 on apoeco:master into e270f41 on gitpoint:master.

@machour
Copy link
Copy Markdown
Member

machour commented Jan 16, 2018

Thank you!!! 🎉 🎉 🎉 🎉 🎉

@machour machour merged commit 17e2a3a into gitpoint:master Jan 16, 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