Code verifications & Lighter demo app #2

aviadhahami · 2016-09-12T11:24:56Z

Hey,
Here's the changelog:

Fixed the path in package.json to relative (rather than local absolute)
Continuing [Example] can not run example app #1 , I've cleaned the linear-gradient and the sparkly deps as they wouldn't link thus project won't compile
some indentation

This change is

…o prevent leaking

…unction params

aviadhahami · 2016-09-12T12:09:36Z

Added few cleanups (unused vars), typos and indentation 👍

jacklam718 · 2016-09-12T14:12:11Z

@aviadhahami I'm use 2 spaces indentation, how many spaces do you indent? The indentation looks many spaces.

aviadhahami · 2016-09-12T14:14:33Z

@jacklam718 i think it's 4 here

jacklam718 · 2016-09-12T14:20:39Z

@aviadhahami Are you used Tab? In my editor looks many spaces.

jacklam718 · 2016-09-12T14:26:51Z

@aviadhahami Do you mind if I change it to 2 spaces? Because I prefer 2 spaces for indentation.

aviadhahami · 2016-09-12T14:28:12Z

sure thing

aviadhahami · 2016-09-12T14:32:00Z

@jacklam718 hope c855a30ffb7fe84272af58785f49880093bce74ais better :)

jacklam718 · 2016-09-12T14:42:56Z

FYI: Sorry i need go to outside now, I will get back to review your PR later. Thank you for PR

jacklam718 · 2016-09-12T18:18:57Z

popupDialogExample/PopupDialogExample.js

 import PopupDialog from 'react-native-popup-dialog';

+const {width, height} = Dimensions.get('window');


I think if the width & height use uppercase will be better.

FYI: have a sugar sytax for ES7:
const { width: WIDTH, height: HEIGHT } = Dimensions.get('window');

then wouldn't be
const { WIDTH:width, HEIGHT:height } = Dimensions.get('window');
?

@aviadhahami Sorry what do you mean?

As in the order of assignment.
WIDTH: width rather than width: WIDTH
FYI: I'm not familiar with this syntactic sugar this I might be wrong.

@aviadhahami
The order of assignment.
{ width: WIDTH, height: HEIGHT }

Cool :)
Can u show me the specs of this ES7 thing? (So I can learn :) )

@aviadhahami
Sorry, It's should belong to ES6.
It called Destructuring Objects / Destructuring Object Property or something like that.

FYI: https://hacks.mozilla.org/2015/05/es6-in-depth-destructuring/

Gotcha :)
Will fix it later today (gym time) ;)

@aviadhahami 👌 👍

jacklam718 · 2016-09-12T18:21:36Z

src/components/Overlay.js


-const SCRREN_WIDTH = Dimensions.get('window').width;
-const SCRREN_HEIGHT = Dimensions.get('window').height;
+const {width, height} = Dimensions.get('window');


Same with PopupDialogExample.js

We can use ES7 sugar sytax & uppercase.

jacklam718 · 2016-09-12T18:50:22Z

@aviadhahami
BTW: I saw you removed spaces inside curly braces. Actually, I follow the Aribnb style guide and on the style guide mentioned the best practice should add spaces inside curly braces.

FYI: https://github.com/airbnb/javascript/blob/master/README.md

The style guide mentioned about this:
18.11 Add spaces inside curly braces. eslint: object-curly-spacing guess: requireSpacesInsideObjectBrackets

// bad
const foo = {clark: 'kent'};

// good
const foo = { clark: 'kent' };

jacklam718 · 2016-09-12T18:53:15Z

@aviadhahami I think all is good 👍, except above things.

aviadhahami · 2016-09-13T09:13:53Z

src/components/Dialog.js

+		setTimeout(() => {
+			dialogState = dialogState === 'closing' ? 'closed' : 'opened';
+			this.setState({dialogState});
+			if (callback && typeof callback === 'function') callback();


@jacklam718 moved the verification here

jacklam718 · 2016-09-13T17:24:55Z

src/PopupDialog.js

+						{...this.props}
+				>
+					{title}
+					{this.props.children} // Critical!


@aviadhahami
Need remove // Critical!, that will make App crash while open dialog.

ok, as long as you keep {this.props.children} intact

jacklam718 · 2016-09-13T17:29:38Z

@aviadhahami
Would you mind if I merge you PR then let me fix above things?

aviadhahami · 2016-09-15T11:06:30Z

No, not at all.
Sorry for not filing it myself I was at a wedding :)

aviadhahami added 17 commits September 12, 2016 13:28

🐛 fixed to relative path

860d37c

🐛 Removed the fancy button + it's deps

6cf4d18

✔️ Done cleaning the demo + designing the button

8c8240d

🚀 Indentation + removed unused variables

4f12a72

🚀 Indentation + removed unused variables

f3039dc

🚀 linting

690e424

🚀 changed child transfer from this.props to this.props.children t…

ee04625

…o prevent leaking

🐛 realized it was my misreading

9c8cc74

🚀 fixed if statement in order to push null rather than undefined

35da565

🚀 fixed lambda assignment

9ed8db6

🚀 Fixed possible issue where onClosed and onOpened might've got non-f…

b20fab3

…unction params

🎨 indentation

44024e1

:bug added explicit constructor just in case

46ebd10

🐛 Typo fix

f8bf822

🐛 fixed unopened dialog

b4ffa6e

🚀 used destructuring rather than explicit assignment

4ab79a3

🎨 Prettyfied

b08001a

🐛 fixed bug where content wasn't presented inside modal

f9efe07

aviadhahami mentioned this pull request Sep 12, 2016

[Critical] No content presented in modal #4

Closed

🎨 padding for inner dialog so text won't touch borders

e1e660c

indents

c855a30

jacklam718 reviewed Sep 12, 2016
View reviewed changes

🚀 move function check from the exported component to the inner logic

e896a21

aviadhahami reviewed Sep 13, 2016
View reviewed changes

🎨 Spaces and linting according to airbnb's specs

ae25237

aviadhahami changed the title ~~Lighter demo app~~ Code verifications & Lighter demo app Sep 13, 2016

jacklam718 reviewed Sep 13, 2016
View reviewed changes

jacklam718 merged commit 17a6a30 into jacklam718:master Sep 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code verifications & Lighter demo app #2

Code verifications & Lighter demo app #2

aviadhahami commented Sep 12, 2016 •

edited by jacklam718

Loading

aviadhahami commented Sep 12, 2016

jacklam718 commented Sep 12, 2016

aviadhahami commented Sep 12, 2016

jacklam718 commented Sep 12, 2016 •

edited

Loading

jacklam718 commented Sep 12, 2016

aviadhahami commented Sep 12, 2016

aviadhahami commented Sep 12, 2016 •

edited

Loading

jacklam718 commented Sep 12, 2016

jacklam718 Sep 12, 2016 •

edited

Loading

aviadhahami Sep 13, 2016

jacklam718 Sep 13, 2016

aviadhahami Sep 13, 2016

jacklam718 Sep 13, 2016

aviadhahami Sep 13, 2016

jacklam718 Sep 13, 2016

aviadhahami Sep 13, 2016

jacklam718 Sep 13, 2016

jacklam718 Sep 12, 2016

jacklam718 commented Sep 12, 2016 •

edited

Loading

jacklam718 commented Sep 12, 2016 •

edited

Loading

aviadhahami Sep 13, 2016

jacklam718 Sep 13, 2016

aviadhahami Sep 15, 2016

jacklam718 commented Sep 13, 2016

aviadhahami commented Sep 15, 2016

		import PopupDialog from 'react-native-popup-dialog';

		const {width, height} = Dimensions.get('window');

Code verifications & Lighter demo app #2

Code verifications & Lighter demo app #2

Conversation

aviadhahami commented Sep 12, 2016 • edited by jacklam718 Loading

aviadhahami commented Sep 12, 2016

jacklam718 commented Sep 12, 2016

aviadhahami commented Sep 12, 2016

jacklam718 commented Sep 12, 2016 • edited Loading

jacklam718 commented Sep 12, 2016

aviadhahami commented Sep 12, 2016

aviadhahami commented Sep 12, 2016 • edited Loading

jacklam718 commented Sep 12, 2016

jacklam718 Sep 12, 2016 • 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

jacklam718 commented Sep 12, 2016 • edited Loading

jacklam718 commented Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacklam718 commented Sep 13, 2016

aviadhahami commented Sep 15, 2016

aviadhahami commented Sep 12, 2016 •

edited by jacklam718

Loading

jacklam718 commented Sep 12, 2016 •

edited

Loading

aviadhahami commented Sep 12, 2016 •

edited

Loading

jacklam718 Sep 12, 2016 •

edited

Loading

jacklam718 commented Sep 12, 2016 •

edited

Loading

jacklam718 commented Sep 12, 2016 •

edited

Loading