Refactor Avatar Size props #1098

Gregjarvez · 2018-04-11T09:46:20Z

codecov · 2018-04-11T09:49:35Z

Codecov Report

Merging #1098 into next will increase coverage by 1.25%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##             next    #1098      +/-   ##
==========================================
+ Coverage   55.57%   56.82%   +1.25%     
==========================================
  Files          33       33              
  Lines         610      586      -24     
  Branches      110      102       -8     
==========================================
- Hits          339      333       -6     
+ Misses        217      205      -12     
+ Partials       54       48       -6

Impacted Files	Coverage Δ
src/avatar/Avatar.js	`16.12% <40%> (+5.71%)`	⬆️
src/icons/Icon.js	`100% <0%> (+7.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d906775...366b76c. Read the comment docs.

xavier-villelegier

I better like this way, much more simple. Thanks @Gregjarvez ! 💯 🔥

Can you please update the docs too ?
And if you feel like a rockstar, you can update the typescript definition too 🎸 😎

xavier-villelegier · 2018-04-11T09:50:24Z

src/avatar/Avatar.js

+  showEditButton: false,
+  onEditPress: null,
+  width: 34,
+  height: 34,


Maybe use your DEFAULT_SIZES object here, to have only one source for the sizes

I will do that @xavier-villelegier 😀typescript 😍

xavier-villelegier

Almost there 💪

xavier-villelegier · 2018-04-11T12:20:55Z

src/avatar/Avatar.js

+  const iconDimension = DEFAULT_SIZES[size];
+
+  if (iconDimension) {
+    [width, height] = iconDimension;


This will fail if size is not one of 'small', 'medium', 'large', 'xlarge' (You can't use this syntax if iconDimension is undefined). You should use the small size as a fallback if the size is not correct (const iconDimension = DEFAULT_SIZES[size] || DEFAULT_SIZES.small).

Of course this messes up your condition if (iconDimension) that will always be true as there is a fallback, so you need to tweak that too to be sure that if the user passes width or height it will be taken instead of default size

I was just about to type this on the way to work :D Good job. Think we can set small as true by default.

Is there any reason we explicitly have height and width as props. If the avatar is either a circle or rectangle that means that width and height will always be the same no? If so size could either be 'small', 'medium', 'large', 'xlarge' or a number.

@Gregjarvez
I think the idea of @iRoachie to have either a string or a number is cool. We can get rid of 6 props and simplify everything.
You can just have something like width = typeof props.size === 'number' ? props.size : DEFAULT_SIZES[size][0] || DEFAULT_SIZES.small[0] (doesn't have to be exactly this but you got the idea), same goes for the height. Be sure you also set small as the defaultProps for size.

Fantastic. so which props do we get rid of apart from width and height

Oh was just talking about 'small', 'medium', 'large', 'xlarge', 'width', 'height'. 6 become 1 props 🔥

oh ok. I will get that done soon !!

@iRoachie @xavier-villelegier I did the changes required seem to work
https://snack.expo.io/rJ_bq9isz
changed code

// iRoachie mentioned avatar is either a circle or a square. one number will work fine const DEFAULT_SIZES = { small: 34, medium: 50, large: 75, xlarge: 150, }; //size is `small` by default in defaultProps let { size } = props; const iconDimension = typeof size === 'number' ? size : DEFAULT_SIZES[size]; let height; let width = (height = iconDimension);

@Gregjarvez Just add a fallback on the small size for iconDimension (DEFAULT_SIZES[size] || DEFAULT_SIZES.small) in case you typed an invalid string size

Otherwise it looks good to me

iRoachie · 2018-04-11T14:32:21Z

docs/avatar.md


 |      Type      | Default |
 | :------------: | :-----: |
-| object (style) |  none   |
+| string(`small`, `medium`, `large`, `xlarge`) |  `small`  |


type would be string(small, medium, large, xlarge) OR number

yeah I changed that for the next pul request

iRoachie · 2018-04-11T14:33:18Z

src/index.d.ts

-   * Extra-large sized icon
-   */
-  xlarge?: boolean;
+  size?: 'small' | 'medium' | 'large' | 'xlarge '


Also needs to update here

xavier-villelegier

👏 👏

iRoachie · 2018-04-11T14:59:19Z

src/avatar/Avatar.js


-  let { width, height } = props;
+  const iconDimension = typeof size === 'number' ? size : DEFAULT_SIZES[size];


You forgot the change from @xavier-villelegier. This will still fail if I pass in size="bob"

oh yeh sorry about that. I was thinking propTypes will fail it
`PropTypes.Oneof(['small', '....'])

default to small even with size="bob"?

Yea but propTypes is just a warning, it doesn't stop the app from trying to load. If we don't provide a fallback it'll red screen (crash)

that is done

iRoachie · 2018-04-11T15:23:33Z

LGTM let's ship it

Monte9

lgtm

iRoachie · 2018-04-13T11:04:56Z

src/index.d.ts

@@ -185,24 +171,10 @@ export interface AvatarProps {
  iconStyle?: StyleProp<TextStyle>;

  /**
-   * Small sized icon
+   * Size of Avatar


Just one small change!

Add a new line here and put @default "small"

@default

* Remove any occurence of `isRequired` #960 * fix(Title) Move disabled style (#1070) * fix(Title) Move disabled style * reorder styles * fix(types): ButtonGroup - Add selectedButtonStyle Replace selectedBackgroundColor with selectedButtonStyle * Run prettier on some stray files * Fix typo in listitem.md (#1078) * Fix typo in listitem.md 'checkbox' corrected to 'checkBox' * fix checkBox typo in versioned docs listitem.md * Allowed Badge to accept any component as the Container component (#1061) * Button group use platform touchable (#1087) * buttongroup platform specific touchable * move component prop logic defaultprops * Add WeChat color to SocialIcon (#1092) * (ListItem) Fix checkmark platform sizing (#1090) * Fix Overlay children typing * docs(Avatar): Add size props small, medium, large, xlarge * docs(ListItem): Change avatar to leftAvatar Add props descriptions to beta4 docs. Fixes #1069 * fix(Avatar): Re-add missing `imageProps` prop * Dismiss overlay by touching backdrop (#1094) * Dismiss Overlay by clicking outside of Overlay Modal * updated overlay docs * rename overlayBackgroundpress to onBackdropPress * updated overlay snapshot * fix overlay prop type * Add `onBackdropPress` to Overlay * Fix Icon style when using TouchableNativeFeedback (#1104) * add encasing view and update test * add encasing view and update test * Refactor Avatar Size props (#1098) * rewrite avatar size prop * updated docs * updated typescript definitions * updated docs, ts and size prop logic * fallback for icon dimension * @default docs * Add raised prop back to Button component (#1108) * Bump docusarus version * docs(website): Fix small bug in listitem docs header * docs(website): Set default version shown as 0.19.0 * docs(website): Replace main icon with svg * Add backdropPress example to docs (#1116) * Use v0.19.0 as default version (#1101) * Revert website default version (#1103) * Use v0.19.0 as default version * Revert "Use v0.19.0 as default version" This reverts commit 1caeefd. * Revert "Use v0.19.0 as default version" This reverts commit 1caeefd. * Add descriptions for ListItem props in v1.0.0-beta4 docs (#1118) * Remove weird duplicate in beta4 docs [ci skip] * button opacity layer border on button press (#1107) * Opacity layer borderRadius on Button press * borderRadius to containerStyle.borderRadius * containerStyle.borderRadius -> buttonStyle.borderRadadius😄 * docs(Card) Button icon needs to be a component (#1132) * Button icon needs to be a component * Use icon element instead of object for Card docs * ci(travis) Pin version of node Node 10 currently breaks builds cause of missing whitelist console in react native jest setup. jestjs/jest#2567 (comment) Pinning node version until react-native/jest bumps * Cleanup definitons [ci-skip] * Bounce Form elements (#1146) * Refactor and make uniform all SearchBars (#1135) * Remove useless flex * Refactor default SearchBar * Add renderIcon helper * Use helper for default SearchBar * Use renderIcon in Input * Add back input icons containers * Improve renderIcon helper * Remove useless import * Use renderIcon for iOS searchbar * Use renderIcon for Android searchbar * Update SearchBar and Input docs * Update this.clear call * Remove useless onCancel on default searchbar * Remove useless hasFocus on default props * Update tests * Fix tests with new props * Use renderNode + refactor nodeType * Fix typo + link to Input props * Use `false` instead of `null` [ci-skip] * docs(Input) Typo it should be labelStyle not labelString * docs(ListItem) onPress component should be TouchableOpacity (#1152) * Typo? It seems that a touchableopacity is added - not a touchablehighlight. shouldn't a touchablehighlight be used here? * https://medium.com/differential/better-cross-platform-react-native-components-cb8aadeba472 * https://stackoverflow.com/questions/39123357/when-to-use-touchablenativefeedback-touchablehighlight-or-touchableopacity thanks! * typo * Fix depth collision with Button and Overlay (#1113) * Update Overlay snapshots * Add Cancel Button Props Object for iOS (#1159) * docs(website): Set default version shown as 0.19.0 (#1156) * ci(travis) Pin version of node (#1158) Node 10 currently breaks builds cause of missing whitelist console in react native jest setup. jestjs/jest#2567 (comment) Pinning node version until react-native/jest bumps * enhancment(searchBar): add 'cancelButtonColor' to search bar component This give a user the abilty to change the color of the text for iOS cancel button * enhancement(searchBar) add 'cancelButtonProps' prop Users can now pass the normal React Native Button props to the cancel button. * Add iOS only to docs * Fix typo in input docs #1041 * Add FormValidationMessage in imports #1144 * (Button) Doc improvement: remove duplicate item (#1167) for 0.19.0 `Button` the `loading` item in props list is duplicate * Create helpers export * Cleanup SearchBar typings and docs (#1169) * (types) Cleanup searchbar definitions * docs(SearchBar): Add inputContainerStyle Closes #1123 * types (SearchBar) SearchBarAndroid should extend from SearchBarPlatform * Fix yarn merge conflict [ci-skip] * Apply renderNode for Button (#1170) * Update docs * Use renderNode helper * Update button icon type * Update types * docs(website): Add Troubleshooting section (#1171) * docs(website): Add Troubleshooting section * Add more complex steps for font issues * Bump version to v1.0.0-beta5 * Create new website version

@default

* rewrite avatar size prop * updated docs * updated typescript definitions * updated docs, ts and size prop logic * fallback for icon dimension * @default docs

xavier-villelegier requested changes Apr 11, 2018

View reviewed changes

Gregjarvez added 3 commits April 11, 2018 11:23

rewrite avatar size prop

cc4c7e2

updated docs

b736966

updated typescript definitions

d7932b1

xavier-villelegier requested changes Apr 11, 2018

View reviewed changes

iRoachie changed the title ~~rewrite avatar size prop~~ Refactor Avatar Size props Apr 11, 2018

iRoachie requested changes Apr 11, 2018

View reviewed changes

iRoachie reviewed Apr 11, 2018

View reviewed changes

updated docs, ts and size prop logic

377d92b

iRoachie approved these changes Apr 11, 2018

View reviewed changes

xavier-villelegier approved these changes Apr 11, 2018

View reviewed changes

iRoachie requested changes Apr 11, 2018

View reviewed changes

fallback for icon dimension

c40065d

iRoachie approved these changes Apr 11, 2018

View reviewed changes

iRoachie added the ✨ Enhancement label Apr 11, 2018

Monte9 approved these changes Apr 13, 2018

View reviewed changes

iRoachie requested changes Apr 13, 2018

View reviewed changes

iRoachie added the 📞 Needs Response from Author label Apr 13, 2018

iRoachie mentioned this pull request Apr 13, 2018

Avatar Placeholder #1076

Closed

@default docs

366b76c

iRoachie removed the 📞 Needs Response from Author label Apr 13, 2018

iRoachie approved these changes Apr 13, 2018

View reviewed changes

iRoachie merged commit fe1edb7 into react-native-elements:next Apr 13, 2018

Gregjarvez deleted the avatar-size-prop branch April 13, 2018 13:34

martinezguillaume pushed a commit that referenced this pull request May 17, 2018

Refactor Avatar Size props (#1098)

e8d95b7

* rewrite avatar size prop * updated docs * updated typescript definitions * updated docs, ts and size prop logic * fallback for icon dimension * @default docs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Avatar Size props #1098

Refactor Avatar Size props #1098

Gregjarvez commented Apr 11, 2018

codecov bot commented Apr 11, 2018 •

edited

Loading

xavier-villelegier left a comment

xavier-villelegier Apr 11, 2018

Gregjarvez Apr 11, 2018

xavier-villelegier left a comment

xavier-villelegier Apr 11, 2018 •

edited

Loading

iRoachie Apr 11, 2018

xavier-villelegier Apr 11, 2018 •

edited

Loading

Gregjarvez Apr 11, 2018

xavier-villelegier Apr 11, 2018

Gregjarvez Apr 11, 2018

Gregjarvez Apr 11, 2018

xavier-villelegier Apr 11, 2018

xavier-villelegier Apr 11, 2018

iRoachie Apr 11, 2018

Gregjarvez Apr 11, 2018

iRoachie Apr 11, 2018

xavier-villelegier left a comment

iRoachie Apr 11, 2018 •

edited

Loading

Gregjarvez Apr 11, 2018 •

edited

Loading

iRoachie Apr 11, 2018

Gregjarvez Apr 11, 2018

iRoachie commented Apr 11, 2018

Monte9 left a comment

iRoachie Apr 13, 2018

Gregjarvez Apr 13, 2018


		let { width, height } = props;
		const iconDimension = typeof size === 'number' ? size : DEFAULT_SIZES[size];

Refactor Avatar Size props #1098

Refactor Avatar Size props #1098

Conversation

Gregjarvez commented Apr 11, 2018

codecov bot commented Apr 11, 2018 • edited Loading

Codecov Report

xavier-villelegier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavier-villelegier left a comment

Choose a reason for hiding this comment

xavier-villelegier Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavier-villelegier Apr 11, 2018 • edited Loading

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

xavier-villelegier left a comment

Choose a reason for hiding this comment

iRoachie Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Gregjarvez Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRoachie commented Apr 11, 2018

Monte9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 11, 2018 •

edited

Loading

xavier-villelegier Apr 11, 2018 •

edited

Loading

xavier-villelegier Apr 11, 2018 •

edited

Loading

iRoachie Apr 11, 2018 •

edited

Loading

Gregjarvez Apr 11, 2018 •

edited

Loading