Skip to content
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

Convert config to ES6 #888

Merged
merged 4 commits into from
Mar 14, 2017
Merged

Convert config to ES6 #888

merged 4 commits into from
Mar 14, 2017

Conversation

ffflorian
Copy link
Contributor

Type of change

Screenshot

// 15 megabyte image upload limit
MAXIMUM_IMAGE_FILE_SIZE: (
15 * 1024 * 1024
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAXIMUM_ASSET_FILE_SIZE: 15 * 1024 * 1024,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes

'image/gif',
],

LOCALYTICS_SESSION_TIMEOUT: (3 * 60 * 1000), // 3 minutes in milliseconds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for parenthesis

MINIMUM_PASSWORD_LENGTH: 8,

// Time until phone code expires
LOGIN_CODE_EXPIRATION: (10 * 60),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for parenthesis

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it makes it more readable.


'use strict';

if (window.z === null) {
Copy link
Contributor

@lipis lipis Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use == instead of === (to include undefined)

PURPLE: 7,
},

// Conversation size
Copy link
Contributor

@lipis lipis Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless comment

// Conversation size
MAXIMUM_CONVERSATION_SIZE: 128,

// self profile image size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments

Copy link
Contributor Author

@ffflorian ffflorian Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it useless? The variable describes only a minimum profile image size, the comment explains that it is about the user's self profile image.

// Maximum of parallel uploads
MAXIMUM_ASSET_UPLOADS: 10,

// Maximum characters per message
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless comment, the constant is self explanatory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because it explains that the length is measured in characters, not in bytes or anything else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we have to update the constant to MAXIMUM_MESSAGE_CHARACTERS

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or something similar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constant renaming comes in another PR...

// 25 megabyte upload limit ( minus iv and padding )
MAXIMUM_ASSET_FILE_SIZE: 25 * 1024 * 1024 - 16 - 16,

// Maximum of parallel uploads
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless comment, the constant is self explanatory

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what iv means.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not self-explanatory. The variable describes only a maximum of any asset uploads, the comment describes that it is about parallel uploads.

MINIMUM_PASSWORD_LENGTH: 8,

// 10 seconds until phone code expires
LOGIN_CODE_EXPIRATION: 10 * 60,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's 10 seconds.. why it's multiplied by 60?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who knows? 😃

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usage of this code is @code_expiration_timestamp z.util.get_unix_timestamp() + z.config.LOGIN_CODE_EXPIRATION.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely 10 minutes - comment is wrong

@ffflorian ffflorian merged commit a61468e into dev Mar 14, 2017
@ffflorian ffflorian deleted the config-js branch March 14, 2017 15:50
@lipis lipis mentioned this pull request Mar 14, 2017
79 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants