refactor: Updated section-list styles to used styled-components by jamesg1 · Pull Request #608 · gitpoint/git-point

jamesg1 · 2017-10-30T10:09:50Z

Question	Response
Version?	v1.4.0
Devices tested?	iPhone 6 and iPhone 8
Bug fix?	yes
New feature?	no
Includes tests?	no
All Tests pass?	yes/no
Related ticket?	#532

Screenshots

Before	After

Description

Converted some of section-list to use Styled-components.

fix: Updated contributors list

jamesg1 · 2017-10-30T10:11:20Z

Please advise on how could this be improved, I did find it confusing trying to convert the View for TopHeader, List and ListItem as styled components don't play nicely with react-native-elements.

Thanks.

coveralls · 2017-10-30T10:24:57Z

Coverage increased (+0.2%) to 39.553% when pulling 49cb192 on jamesg1:feature/migrate-sectionlist-styled-comp into 753f60c on gitpoint:master.

chinesedfan

@jamesg1 Good job! Glad to see that you notice RNE problems, @machour will try to fix it.

chinesedfan · 2017-10-30T13:24:20Z

src/components/section-list.component.js

 };

+const Section = styled.View`
+  margin-top: 15px;


Please keep unitless for simple properties.

I changed it to margin: 15 0 0 and this didn't work? What do you mean by unitless? We are writing css within styled components so margin: 15 or margin-top: 15 isn't valid.

@jamesg1 If you use unitless shorthands properties in styled components, like margin: 15 0 0 or margin: 15, it will crash. But for simple properties, like margin-top: 15, it is OK.

I have to admit that it is a little ambiguous. Real CSS needs units, while RN doesn't need. But how about styled components in RN? For now, we prefer RN more.

chinesedfan · 2017-10-30T13:25:15Z

src/components/section-list.component.js

+  padding: 15px;
+`;
+const LoadingIcon = styled(ActivityIndicator)`
+  margin: 20px 0px;


Of course, shorthands properties need units, but except for 0. So here can be changed as margin: 20px 0;.

chinesedfan · 2017-10-30T13:26:51Z

.all-contributorsrc

+      "name": "James G",
+      "avatar_url": "https://avatars3.githubusercontent.com/u/3621147?v=4",
+      "profile": "https://github.com/jamesg1",
+      "contributions": []


You can select code as your contributions. :)

jamesg1 · 2017-10-31T11:31:59Z

@chinesedfan I've made those requested changes, Thanks

coveralls · 2017-10-31T11:41:19Z

Coverage increased (+0.3%) to 44.511% when pulling cab7e08 on jamesg1:feature/migrate-sectionlist-styled-comp into 0eef067 on gitpoint:master.

machour · 2017-11-04T19:29:37Z

Hey @jamesg1! react-native-elements 0.18.1 was published yesterday and I'm working on a PR to upgrade to it and fix all the problems introduced with styled components.

Putting this PR on hold until I'm done, then I'll be happy to help you with this one if needed

machour

Hey @jamesg1, we now use the new react-native-elements version which plays way nicer with styled-components 👌

Left you a comment to show you how to style one of them for example.

Could you take care of converting the remaining old styles to styled components? Let me know if you need any help with that!

machour · 2017-11-07T19:19:20Z

src/components/section-list.component.js

+    listDisplay = <LoadingIcon animating={loading} />;
  } else if (noItems) {
    listDisplay = (
      <ListItem


Can now be styled like this:

const SectionListItem = styled(ListItem).attrs({ titleStyle: { color: colors.black, ...fonts.fontPrimary, } })``;

Thanks @machour I'll take a look, I was on vacation.

@machour Updated code accordingly, check it out 👍

…tionlist-styled-comp # Conflicts: # .all-contributorsrc # CONTRIBUTORS.md # README.md # src/components/section-list.component.js

jamesg1 · 2018-02-19T15:01:32Z

@machour @chinesedfan Sorry for the delay updated this PR with some changes 👍

chinesedfan

@jamesg1 Welcome back! But I am afraid you need to re-run your codes and update the screenshots comparison.

chinesedfan · 2018-02-19T15:52:14Z

src/components/section-list.component.js

+  containerStyle: {
+    margin-top: 0;
+    border-bottom-color: ${colors.grey},
+    border-bottom-width: 1,


Those attributes should be object style, instead of css style.

chinesedfan · 2018-02-19T15:52:45Z

src/components/section-list.component.js

+const TitleView = styled.View`
+  padding: 15px;
+`;
+const SectionList = styled(List).attrs({


SectionList is already used as exported component name.

chinesedfan · 2018-02-19T15:53:16Z

src/components/section-list.component.js

+const SectionListItem = styled(ListItem).attrs({
+  titleStyle: {
+     color: colors.black,
+      ${fonts.fontPrimary},


Object grammer here.

chinesedfan · 2018-02-19T15:54:25Z

src/components/section-list.component.js

+  border-color: ${colors.primaryDark};
+  border-width: 1;
+  border-radius: 3;
+  padding: 5px 10px;


These fields should be attr buttonStyle.

chinesedfan · 2018-02-19T15:55:10Z

src/components/section-list.component.js

              textStyle={fonts.fontPrimarySemiBold}
              fontSize={13}
              color={showButton ? colors.primaryDark : colors.white}
-              buttonStyle={styles.button}


Besides buttonStyle, please move other props like textStyle/fontSize/color to styled-components attrs, too.

Waiting on updated screenshots to perform a new review

chinesedfan

@jamesg1 I am not sure whether you are ready. I leave some comments to remind.

chinesedfan · 2018-02-21T11:09:32Z

src/components/section-list.component.js

+  margin-top: 15;
+`;
+const Header = styled.View`
+  flex: auto;


Does flex: auto the same with flex: 1?

I couldn't get flex: 1 to work it would result in the content below overlapping the header.

Hmm, the original code uses flex: 1. Never mind, maybe you discovered a bug and fixed. We can help you check it.

This is what I get with flex: 1.
![flex 1 screenshot](https://user-images.githubusercontent.com/3621147/36538300-9be10206-181e-11e8-8c23-15615282c553.png =250x)

flex: 0 seemed to fix the flexbox issues.
![flex 0 screenshot](https://user-images.githubusercontent.com/3621147/36538413-10f191d2-181f-11e8-9c09-a4ff5ef18e36.png =250x)

Also flex-grow: 0; flex-basis: 0; also looked good.

I've gone with flex-grow: 0; flex-basis: 0;

@jamesg1 Interesting. flex: 1 of styled-components really results in weird UI, while the original code works well.

But the initial values are flex-grow: 0; flex-shrink: 1; flex-basis: auto;. You can delete flex-grow: 0; flex-basis: 0;.

I deleted flex-grow: 0; flex-basis: 0; on Header styling.
Thanks

chinesedfan · 2018-02-21T11:13:21Z

src/components/section-list.component.js

    backgroundColor: colors.white,
    borderColor: colors.primaryDark,
    borderWidth: 1,
-    borderRadius: 3,


Why removing this line?

Yeah sorry was a typo.

chinesedfan · 2018-02-21T11:14:47Z

src/components/section-list.component.js

-    borderRadius: 3,
    paddingVertical: 5,
    paddingHorizontal: 10,
+    ...fonts.fontPrimaryBold,


fonts.fontPrimaryBold belongs to attr textStyle, and other attrs(fontSize/color) are missing.

color={showButton ? colors.primaryDark : colors.white} didn't make sense as showButton will always be true if we are showing the button as its checked before rendering the button showButton &&. Would colour go in buttonStyle or textStyle?

I tried to get fontSize into buttonStyle initially. Will move it to textStyle

Would colour go in buttonStyle or textStyle?

Neither. It belongs to an attr iteself, as well as fontSize.

const StyledButton = styled(Button).attrs({ textStyle: {}, fontSize: 13, color: colors.primaryDark, buttonStyle: {}, }``;

Yeah awesome made the change!

jamesg1 · 2018-02-22T12:55:39Z

Updated PR with styling changes and screenshots. Thanks 👍

machour · 2018-02-22T13:00:13Z

@jamesg1 the screenshots looks weird in the header (time overlapping with your avatar) is that when you're scrolling down?

jamesg1 · 2018-02-22T13:26:26Z

Yeah its because its scrolled. Good pickup I'll fix them now.

machour · 2018-02-23T08:07:31Z

Working as expected on Android 🎉

fix: Updated section-list styles to used styled-components

49cb192

fix: Updated contributors list

jamesg1 changed the title ~~fix: Updated section-list styles to used styled-components~~ refactor: Updated section-list styles to used styled-components Oct 30, 2017

chinesedfan requested changes Oct 30, 2017

View reviewed changes

fix: Refactored section list and updated contributors

94672b4

chinesedfan added the 💅 styled-components label Nov 1, 2017

alejandronanez mentioned this pull request Nov 1, 2017

Migration to styled-components #532

Open

51 tasks

machour added the on hold label Nov 4, 2017

Merge branch 'master' into feature/migrate-sectionlist-styled-comp

a5282b1

machour added 🏗️ work in progress and removed on hold labels Nov 7, 2017

machour previously requested changes Nov 7, 2017

View reviewed changes

jamesg1 added 5 commits February 19, 2018 23:53

Merge remote-tracking branch 'origin/master' into feature/migrate-sec…

5beb9b6

…tionlist-styled-comp # Conflicts: # .all-contributorsrc # CONTRIBUTORS.md # README.md # src/components/section-list.component.js

Updated section-list with changes after merge

5f5a6e1

Updated styling for Section list to use styled_component

c4a3d22

Updated section list styling

7282f03

Added self to contributions list and updated section-list component

09ce9d1

chinesedfan requested changes Feb 19, 2018

View reviewed changes

feat: Refactor of section-list stylesheet to styledComponents

87b04f2

chinesedfan requested changes Feb 21, 2018

View reviewed changes

feat: StyledButton and Header flex style fix

a20dadc

chinesedfan added ready for review and removed 🏗️ work in progress labels Feb 22, 2018

feat: Removed default flexbox styles from header

cab7e08

chinesedfan approved these changes Feb 23, 2018

View reviewed changes

machour merged commit 8c3d75c into gitpoint:master Feb 23, 2018

Conversation

jamesg1 commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Description

Uh oh!

jamesg1 commented Oct 30, 2017

Uh oh!

coveralls commented Oct 30, 2017

Uh oh!

chinesedfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesg1 Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesg1 commented Oct 31, 2017

Uh oh!

coveralls commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

machour commented Nov 4, 2017

Uh oh!

machour left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesg1 commented Feb 19, 2018

Uh oh!

chinesedfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinesedfan Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinesedfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesg1 Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jamesg1 commented Oct 30, 2017 •

edited

Loading

jamesg1 Oct 31, 2017 •

edited

Loading

coveralls commented Oct 31, 2017 •

edited

Loading

chinesedfan Feb 19, 2018 •

edited

Loading

jamesg1 Feb 22, 2018 •

edited

Loading

jamesg1 Feb 21, 2018 •

edited

Loading