Skip to content

refactor(diff-blocks): convert diff-blocks to styled component#533

Closed
machour wants to merge 3 commits intogitpoint:masterfrom
machour:diff-blocks-styled
Closed

refactor(diff-blocks): convert diff-blocks to styled component#533
machour wants to merge 3 commits intogitpoint:masterfrom
machour:diff-blocks-styled

Conversation

@machour
Copy link
Copy Markdown
Member

@machour machour commented Oct 21, 2017

My first converted component!

I've found the need to define a new config.fontsStyled object with converted styles rules.
Is that ok?

Also renamed styles to avoid colors in names.

fontCode: { fontFamily: 'Menlo' },
};

export const fontsStyled = {
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.

I think @josenaranjo is working on something related to fonts as he mentioned here AFAIK he created a new file called styledFonts.js inside /config, maybe you can talk to him before merging this PR so you don't get any merge conflict in the future. FWIW I like the idea of having a new fresh file with fonts.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

styledFonts.js looks good to me. I'll wait on @josenaranjo PR to get merged before changing this one

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.

@machour ok, @josenaranjo's PR was merged #561 You can start using his font file.

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.

🤔 I actually like having all the fonts in one file. Not sure though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

seconding @andrewda, keeping both variables in the same file will help making sure we change both when updating something.

@alejandronanez Is it okay of I merge it back in fonts.js ?

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.

Sounds good to me, do you mind removing the file @josenaranjo added on his PR and doing a small refactor on his component so we don't introduce more tech debt in here?

Copy link
Copy Markdown
Member

@alejandronanez alejandronanez left a comment

Choose a reason for hiding this comment

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

Let's talk to @josenaranjo about the fonts file he created for #515 before merging. Besides that LGTM!

@andrewda andrewda changed the title chore(*): convert diff-blocks.component to styled component refactor(diff-blocks): convert diff-blocks to styled component Oct 21, 2017
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.4%) to 34.943% when pulling 9545fbe on machour:diff-blocks-styled into 6567aa3 on gitpoint:master.

@machour
Copy link
Copy Markdown
Member Author

machour commented Jan 14, 2018

Replaced by #692

@machour machour closed this Jan 14, 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.

4 participants