Add different text components and add them in the example app #38

ahmedlhanafy · 2016-10-24T03:15:13Z

No description provided.

satya164 · 2016-10-24T11:00:40Z

src/Typography/Caption.js

+import fonts from '../styles/fonts';
+import withTextProps from './withTextProps';
+
+const Subheading = withTextProps(fonts.regular, 12);


Any reason you prefer a HOC here instead of a plain component?

It was a lot of repetitive work when we only need to change the fontFamily and fontSize so creating HOC reduces this boilerplate a lot

it's just
const Subheading = props => <StyledText {...props} family={fonts.regular} size={12} />
instead of
const Subheading = withTextProps(fonts.regular, 12);

That would make more sense 👍

I like component here just because the props are named unlike function arguments, and we could add more props later rather than adding more arguments which is confusing.

btw just realized, this is not HOC, it's just a function which returns a component.

probably rename it to createStyledText or something like that. withTextProps make it sound like an HOC. But it doesn't take a component as an argument, so it is basically creating a new component with the text styles you give. does it make any sense?

Yeah, I knew that the name is misleading couldn't think of a better one at 5:30 in the morning :D

satya164 · 2016-10-24T11:01:52Z

src/styles/fonts.js

+  Platform,
+} from 'react-native';
+
+const isAndroid = Platform.OS === 'android';


should probably be isIOS, i.e. you set font to helevetica if iOS, other wise sans-serif

Why is that does sans-serif works on WP?

@ahmedlhanafy it should, it's just system's sans-serif

but my thought was that helvetica is iOS specific, sans-serif isn't android specific, so the other logic makes more sense

Makes sense

satya164 · 2016-10-24T11:07:55Z

src/styles/DefaultTheme.js

 } from './colors';

 export default {
  colors: {
    primary: indigo500,
    primaryDark: indigo700,
    accent: pinkA200,
+    primaryText: black,


probably we need to think bit more about the naming. We can't name them as primaryText etc. since it signifies connection with primaryColor. I guess we also need a primaryText which will be the text written on a primary color background (or may be think of a better name).

So probably bodyText (or even just text) is a better name?

Where will secondaryText and disabledText be used? Generally buttons and controls are disabled, not text?

probably we could just use normal text color and use opacity when needed instead of adding options in the theme? not sure. I'm saying this because otherwise we'll probably end up adding configs for each component. and people can customize it anyways by passing a style prop to individual components.

The thing is this is the way MD Docs refers to text in the rest of the docs, primaryText,secondaryText and disabledText. We want to make it easy for people contributing to remember these constants.
I think secondaryText and disabledText have their uses across the docs.

@satya164

textColor: fullWhite, secondaryTextColor: fade(fullWhite, 0.7),

This is how MUI is doing it and they have disabled as well, I think this is a better approach than naming things body...etc. It makes things clearer in my opinion

@ahmedlhanafy

The thing is this is the way MD Docs refers to text in the rest of the docs

I immediately associate it with primaryText with primaryColor though

I think secondaryText and disabledText have their uses across the docs

can you give an example, I don't get how a text can be disabled coz it's not an interactive element.

This is how MUI is doing it and they have disabled as well

textColor is ok, that's why I suggested bodyText or text. But I don't wanna put disabledText in the theme config without having a use case. Probably we should put configs in theme when we start using them?

@satya164 For the disabled text here check this link and see how it's used.
we can rename it to textColor and secondaryTextColor

@ahmedlhanafy Hmm... but then it's not actually disabled text, but anything which is disabled, probably disabledText isn't the right name

@ahmedlhanafy also what's secondaryText?

@ahmedlhanafy I think mui is doing it right by naming it as disabledColor and not disabledTextColor

Check MD Docs you will find references to it.
we can rename it to disabled instead of disabledText then

satya164 · 2016-10-24T11:13:46Z

src/styles/DefaultTheme.js

@@ -1,15 +1,19 @@
-// @flow
-
+/* @flow */


nit: new line after

satya164 · 2016-10-24T11:14:40Z

index.js

@@ -3,6 +3,13 @@
 export { default as ThemeProvider } from './src/core/ThemeProvider';
 export { default as withTheme } from './src/core/withTheme';

-export * as Colors from './src/styles/colors.js';
+export * as Colors from './src/styles/colors';
+export * as Fonts from './src/styles/fonts';


should we export this? do you see any use case where it'll be used except the default theme?

I think we will need it, it's nice to have these constants so that the user can benefit from them.

@ahmedlhanafy can you give an example? I can't think of a case where this will be useful

also people can always access it via the default theme object which we should export, if people wanna make minor changes to default theme.

I think they should use our prebuilt components rather than these constants, I will remove that

…S in fonts

Add different text components and add them in the example app

a51266d

ahmedlhanafy mentioned this pull request Oct 24, 2016

Text #23

Closed

satya164 reviewed Oct 24, 2016

View reviewed changes

ahmedlhanafy added 2 commits October 24, 2016 14:55

Rename textWithProps to createStyledText and change isAndroid to isIO…

24dc154

…S in fonts

Rename primaryText to text and disabled

481254d

satya164 merged commit 28313b2 into master Oct 24, 2016

satya164 deleted the text-component branch October 24, 2016 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add different text components and add them in the example app #38

Add different text components and add them in the example app #38

ahmedlhanafy commented Oct 24, 2016

satya164 Oct 24, 2016

ahmedlhanafy Oct 24, 2016

satya164 Oct 24, 2016 •

edited

ahmedlhanafy Oct 24, 2016

satya164 Oct 24, 2016

satya164 Oct 24, 2016

ahmedlhanafy Oct 24, 2016

satya164 Oct 24, 2016

ahmedlhanafy Oct 24, 2016

satya164 Oct 24, 2016

ahmedlhanafy Oct 24, 2016

satya164 Oct 24, 2016

ahmedlhanafy Oct 24, 2016

ahmedlhanafy Oct 24, 2016

satya164 Oct 24, 2016

ahmedlhanafy Oct 24, 2016 •

edited

satya164 Oct 24, 2016

satya164 Oct 24, 2016

satya164 Oct 24, 2016

ahmedlhanafy Oct 24, 2016

satya164 Oct 24, 2016

satya164 Oct 24, 2016

ahmedlhanafy Oct 24, 2016

satya164 Oct 24, 2016

satya164 Oct 24, 2016

ahmedlhanafy Oct 24, 2016 •

edited

Add different text components and add them in the example app #38

Add different text components and add them in the example app #38

Conversation

ahmedlhanafy commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satya164 Oct 24, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedlhanafy Oct 24, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedlhanafy Oct 24, 2016 • edited

Choose a reason for hiding this comment

satya164 Oct 24, 2016 •

edited

ahmedlhanafy Oct 24, 2016 •

edited

ahmedlhanafy Oct 24, 2016 •

edited