Skip to content

Convert to styled-components #503

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

Closed
lex111 opened this issue Oct 18, 2017 · 15 comments
Closed

Convert to styled-components #503

lex111 opened this issue Oct 18, 2017 · 15 comments

Comments

@lex111
Copy link
Member

lex111 commented Oct 18, 2017

After discuss in the issue #480, we begin the gradual migration of the codebase to styled-components. This is a good and easy task for those who want to participate in Hacktoberfest or just start contributing to the project. The only rule is: one component or screen converted to styled-components in your PR.

You can see the example of migration in PR #495.

@agatac
Copy link

agatac commented Oct 18, 2017

Hi, I'm a new contributor. I can start with Badge Component.

@lex111
Copy link
Member Author

lex111 commented Oct 18, 2017

@agatac fine, we will wait for PR, if there are problems, please contact to chat on Gitter.

@housseindjirdeh
Copy link
Member

Agreed @lex111, I love the one component rule for now 🙌

Welcome @agatac! The badge component is a great place to start :)

As per our discussion in #480, we can keep those styles in the same file for now. We can definitely look into moving them into separate files at a later point if need be. Any questions you have at any time, please don't hesitate to ask 🙏

@nimadera
Copy link

I'd like to help with this. Should I choose a component or is there a recommended component to tackle next?

@lex111
Copy link
Member Author

lex111 commented Oct 19, 2017

@nimadera no, you can choose any component you like, just write it here so that others know about it.

pdong added a commit to pdong/git-point that referenced this issue Oct 19, 2017
Convert notification-icon.component.js to use styled-components as part of gitpoint#503
@pdong
Copy link
Contributor

pdong commented Oct 19, 2017

I took a shot at the view container, label-list-item, and notification icon styles. I went through some other ones but got stuck at some spots or wasn't sure where the components were used. I'm not sure if there is an easy way to reload certain views either as I'm pretty new to react native.

A couple of notes:
For fonts, the project is currently bringing them in as a spread operator in the StyleSheet. I'm assuming we'd now be importing them as ${fonts.fontPrimarySemiBold.fontFamily} (I looked at the config and fontFamily was the only property so this seems safe).

For a component like label-button-component which takes props to conditionally apply styles, I wasn't really sure what approach should be made here. The props can be passed into the styled container (e.g. ${( { largeWithTag }: Props) => {}}) but I was thinking an approach like

const ButtonContainer = styled(Button).attrs({
  'padding-right': props => props.large ? 10 : 5,
  'padding-left': props => props.large ? 10 : 5,
  'padding-top': props => props.large ? 5 : 3,
  'padding-bottom': props => props.large ? 5 : 3,
  'margin-right': props => props.large ? 0 : 10,
  'min-width': props => props.large ? 'unset' : 70,
})`
  border-radius: 3;
  margin-left: 0;
  padding-right: ${props => props['padding-right']}
  padding-left: ${props => props['padding-left']}
  padding-top: ${props => props['padding-top']}
  padding-bottom: ${props => props['padding-bottom']}
  margin-right: ${props => props['margin-right']}
  min-width: ${props => props['min-width']}
`;

might be cleaner. I wasn't able to get this working though as I ran into some issues with my simulator before I could test so I don't know if I'm on the right track. Some input would be great there.

Also I took a stab at repository-section-title. I'm used to being able to do something like const Badge = styled(StateBadge) but for some reason it's telling me StateBadge is an undefined component when I reload. Not sure if this is a react-native thing or how to get around it as I'm pretty new to react-native.

Anyway let me know if I'm on the right track with my current PRs. Thanks 👋

@agatac
Copy link

agatac commented Oct 19, 2017

Unfortunately I'm unable to configure my dev environment. It took too much time so far and I have no experience with React Native so I'm giving up, sorry guys! :/

@josenaranjo
Copy link
Contributor

Hello, I'm a new contributor and I'm working in the code-line.component.js.

@pdong for the fonts I created a new file called styledfonts.js under the config folder and this properties file has only the names of the fonts. I think it would be better if we start with a fresh font file while we migrate all the components, later we can deprecate/delete fonts.js

Also, I've noticed that some component's styles are handled through style arrays (override mechanism), which makes a little bit trickier this migration. I need to analyze better this scenario and will post here again once I have an idea on how to solve it. I hope we can discuss it.

Thanks

@alejandronanez
Copy link
Member

Hey @josenaranjo, welcome to the project and thanks for working on this. I agree with your idea of starting fresh with our fonts.
Do you mind creating an issue or a WIP PR that we can start looking at your progress?

housseindjirdeh pushed a commit that referenced this issue Oct 20, 2017
Convert notification-icon.component.js to use styled-components as part of #503
@housseindjirdeh
Copy link
Member

housseindjirdeh commented Oct 20, 2017

No worries @agatac, set up always is a hassle 😞 Please let us know anytime if you ever feel like trying again and we'll always be here to help

@pdong I personally prefer if props can be passed in just like you suggested :) Seems like a cleaner way to do things!

@josenaranjo Interesting, will like to see your approach as well. If having a separate fonts file makes things easier than definitely on board with that

dyesseyumba pushed a commit to dyesseyumba/git-point that referenced this issue Oct 20, 2017
…int#510)

Convert notification-icon.component.js to use styled-components as part of gitpoint#503
@davidsonsns
Copy link

Hi, I'm new here too, I believe that implementing the styled components would be a good way to get in.

Since there are several components, I think it's okay if I help. To avoid problems, can I open another issue with the name of the component or just say here?

@chinesedfan
Copy link
Member

In v2 we support percentages. To make this possible we need to enforce units for all shorthands. If you're migrating to v2, a codemod is available.

@lex111 @housseindjirdeh I found the latest master will crash due to above styled components restriction. Please pay attentions to this.

@lex111
Copy link
Member Author

lex111 commented Oct 21, 2017

@chinesedfan where does the crash happen? I do not have one.

@chinesedfan
Copy link
Member

@lex111 It has already been fixed by 133091.

@lex111
Copy link
Member Author

lex111 commented Oct 21, 2017

Allright, then I close in favor of #532

@lex111 lex111 closed this as completed Oct 21, 2017
chinesedfan pushed a commit that referenced this issue Jan 6, 2018
* refactor(fonts): Remove useless fonts in android (#485)

* refactor(fonts): Remove MaterialIcons from used fonts in android (#485)

BREAKING CHANGE: Update link script in Package.json

* Revert "refactor(fonts): Remove MaterialIcons from used fonts in android (#485)"

This reverts commit 282f475.

* fix: Update stateRandom and reset cookies after a successful login (#494)

* feat(markdown): Add support for quoted emails (#501)

* feat(markdown): Add support for quoted emails

* fix: use paddingHorizontal instead of Left and Right

* refactor: Drop rn-app-intro in favor of react-native-swiper (#493)

* refactor: Drop rn-app-intro in favor of react-native-swiper

* fix: Don't embed swiper in a View, so that it works on Android

* chore: Hide the commitlint folder (#488)

* feature(translation): add Spanish translation (#442)

* Spanish file (first translation)

* Languague related files

* Fix

* Replaced double quotes

* Improved spanish translation

* Minor improvements (spanish translation)

* Some improvements (spanish translation)

The GIT reserved words are kept without translate.

* Removed english phrase

* Updated analytics title

* Updated spanish translation

* Added missing fields (spanish translation)

* Replaced tabs (spanish translation)

* Changed some words to downcase (spanish translation)

* feat: Issue Events (#438)

* feat(issue_events): Show events on issues

* style(issue_events): Added styles to "added label" event

* style(issue_events): Add icon & improve styling of added labels

* style(issue_events): Improve <ReviewRequested /> styles

* feat(issue_events): Remove mentioned/subscribed events from UI

* feat(issue_events): Define <Closed /> events

* refactor(issue_events): Extract <EventIcon /> and <Date />

* feat(issue_events): Add `unlabled` prop to <Labeled />

* feat(issue_events): Define <Merged /> event

* feat(issue_events): Filter out `closed` events preceded by `merged`

* feat(issue_events): Define <HeadRef /> events

* feat(issue_events): Define <Assigned /> events

* feat(issue_events): Define <Reopened /> & <Renamed /> events

* refactor(issue_events): Render <Text /> from <ActorLink />

* style(issue_events): Trim issue names to ensure spacing

* feat(issue_events): Define <Locked />  event

* feat(issue_events): Define <Milestoned />  event

* refactor(issue_events): Clean up authUser from LabeledComponent

* feat(issue_events): Define <MarkedAsDuplicate /> event

* refactor(issue_events): Define generic <Event /> component

* docs(readme): Add @brandly as a contributor

* feat(issue_events): Define <LabelGroup /> for list of label changes

* refactor(issue_events): Use spread operator for textChildren

* style(issue_events): Add blank line after external imports

* feat(issue_events): <InlineLabel /> has rounded corners

* refactor(issue_events): Move <InlineLabel /> into own file

* feat(issue_events): Press username in events to view profile

* refactor(events): Inline most <Event />s into <IssueEventListItem />

* refactor(events): Eliminate <Date /> since its only used once

* refactor(events): Extract formatEventsToRender into event-helpers

* fix(ux): Add back button for AuthProfileScreen (#507)

Adds back button when AuthProfileScreen is not the root of a StackNavigator.

Ps. AuthProfileScreen is StackNavigator root when the routeName is MyProfile.

* style(issueeventlistitem, commentlistitem): Slightly shrink issue event badges + change user click o (#516)

* fix: Remove undefined var & fix typo (#517)

* chore: fix `yarn run link` (#513)

* chore(fonts): Not link all fonts from react-native-vector-icons

* fix(fonts): Add the missing Menlo

* fix(cli): Fix for RN 0.48

* chore(cli): Run `yarn run link` again

* chore(*): convert notification icon styles to styled-component (#510)

Convert notification-icon.component.js to use styled-components as part of #503

* chore(*): convert label-list-item component styles to styled component (#509)

Converted the label-list-item component styles to styled-components

* chore(*): style view-container.component.js (#508)

Converted view-container.component.js to use styled components

BREAKING CHANGE: none

none

* refactor(auth): get user data after login (#502)

* test: begin implementing basic component tests (#407)

* test: begin implementing tests

* test: add more tests

* test: add Buton and Badge tests

* test: add StateBadge test

* test: only import used enzyme wrappers

* chore(deps): update table component to 1.1.0

* test: Add tests for ToggleView

* Update ToggleView.js

* refactor(fonts): Remove useless fonts in android (#485)

* refactor(fonts): Remove MaterialIcons from used fonts in android (#485)

BREAKING CHANGE: Update link script in Package.json

* Revert "refactor(fonts): Remove MaterialIcons from used fonts in android (#485)"

This reverts commit 282f475.

* test: Add tests for CommentInput (#518)

* refactor: Beautify the code of CommentInput unit test.

* refactor: use jest mocks instead of sinon spies.

* refactor: Apply @chinesedfan recommendations

* refactor: Improve test descriptions.

* test: Add two cases of test and integrate styled-components in tests.

Add two more cases for userHasPushPermission and issueLocked. Update test to use migrated components
to styled-components.

* refactor: Remove console.log statement.

* test: Improve test descriptions and remove useless console.log

* test: Fix conflicts between descriptions and implementations.

* refactor: Runned prettier on CommentInput.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants