Skip to content

Conversation

mymindstorm
Copy link
Member

No description provided.

@mymindstorm mymindstorm requested a review from Sneezry July 11, 2019 15:27
@Sneezry
Copy link
Member

Sneezry commented Jul 11, 2019

zh_CN/message.json should be updated as well. Maybe we should move zh_CN back to crowdin

@mymindstorm
Copy link
Member Author

zh-cn is on crowdin now

const entry = new OTPEntry({
if (type === OTPType.hhex || type === OTPType.hotp) {
this.newAccount.period = NaN;
Copy link
Member

Choose a reason for hiding this comment

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

NaN may not be a good default value here. If period is not available for specific types, null or undefined is a better option.

Copy link
Member Author

Choose a reason for hiding this comment

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

this.newAccount.period is typed as a number and it wouldn't let me set it as null. NaN evaluates to false so it will just treat it as if period was set to null. Is there a good reason not to use NaN?

Copy link
Member

@Sneezry Sneezry Aug 8, 2019

Choose a reason for hiding this comment

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

NaN's meaning is Not a Number. When you regard something not a number as a number (such as using Number function), its value will be set to NaN. Here, hhex or hotp doesn't has period property, but not has period of which value is not a number. null is all zero in bits in memory, which means nothing, or doesn't exist. We can change type of this.newAccount.period to number | null.

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.

2 participants