Skip to content

Conversation

@CatWithApple
Copy link
Contributor

@CatWithApple CatWithApple commented Mar 27, 2019

in "ru-ru" locale
Screenshot 2019-03-27 at 16 40 19

in "en-us" locale
Screenshot 2019-03-27 at 16 40 25

@CatWithApple CatWithApple requested review from DudaGod, rostik404 and sipayRT and removed request for DudaGod March 27, 2019 13:33
retries: PropTypes.number.isRequired
})
}),
date: PropTypes.string.isRequired
Copy link
Member

Choose a reason for hiding this comment

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

почему оно обязательное то?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

просто потому, что мы контролируем это в dateToLocaleString

<SummaryKey label="Failed" value={failed} isFailed={true}/>
<SummaryKey label="Skipped" value={skipped}/>
<SummaryKey label="Retries" value={retries}/>
<span className='summary__date'>created in {date}</span>
Copy link
Member

Choose a reason for hiding this comment

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

разве не created at?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

да, конечно.

Copy link
Member

Choose a reason for hiding this comment

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

почему span а не div, если ты его все равно флоатишь в правую часть экрана?

Copy link
Contributor

Choose a reason for hiding this comment

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

почему span а не div, если ты его все равно флоатишь в правую часть экрана?

И что?

Copy link
Member

Choose a reason for hiding this comment

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

да ничего ) пытаюсь понять почему был использован данный html тег

return resStats;
}

function dateToLocaleString(date) {
Copy link
Member

Choose a reason for hiding this comment

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

кажется, ты здесь немного упоролся - давай просто время с AM/PM оставим и все

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Проблема в основном с положением месяцев относительно дней. Нашим пользователям, полагаю, будет удобнее DD.MM.YYYY, а по дефолту MM/DD/YYYY.

new Date().toLocaleString()
"3/28/2019, 1:18:26 PM"

new Date().toLocaleString('en-US')
"3/28/2019, 1:18:35 PM"

new Date().toLocaleString('en-GB')
"28/03/2019, 13:18:38"

new Date().toLocaleString('ru')
"28.03.2019, 13:18:42"

Copy link
Member

Choose a reason for hiding this comment

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

а мне все таки нравится. Я постоянно забываю PM это утро или вечер. Плюс неудобно читать, что дата вторым значением указана. Го голосовать ;)

Copy link
Member

Choose a reason for hiding this comment

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

Я постоянно забываю PM это утро или вечер

рукалицо

Copy link
Member

Choose a reason for hiding this comment

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

кулакподбородок

Copy link
Member

@DudaGod DudaGod left a comment

Choose a reason for hiding this comment

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

/ok

<SummaryKey label="Failed" value={failed} isFailed={true}/>
<SummaryKey label="Skipped" value={skipped}/>
<SummaryKey label="Retries" value={retries}/>
<span className='summary__date'>created in {date}</span>
Copy link
Member

Choose a reason for hiding this comment

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

почему span а не div, если ты его все равно флоатишь в правую часть экрана?

return {
stats: statsToShow
stats: statsToShow,
date
Copy link
Member

Choose a reason for hiding this comment

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

я бы одной строкой выводил, тут же всего 2 поля


function dateToLocaleString(date) {
if (!date) {
return '';
Copy link
Member

Choose a reason for hiding this comment

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

а в каком случае мы можем в это условие попасть? Кажется его выгасить можно

Copy link
Contributor Author

Choose a reason for hiding this comment

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

если в gui режиме

return resStats;
}

function dateToLocaleString(date) {
Copy link
Member

Choose a reason for hiding this comment

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

а мне все таки нравится. Я постоянно забываю PM это утро или вечер. Плюс неудобно читать, что дата вторым значением указана. Го голосовать ;)

lang = navigator.languages[0];
} else {
lang = navigator.language;
}
Copy link
Member

Choose a reason for hiding this comment

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

Я бы так написал:

const lang = _.isEmpty(navigator.languages) ? navigator.language : navigator.languages[0];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

норм

@CatWithApple CatWithApple force-pushed the report-creation-date branch from 3465578 to 80c18c7 Compare March 28, 2019 10:39
@CatWithApple CatWithApple force-pushed the report-creation-date branch from 80c18c7 to e84757e Compare March 28, 2019 11:01
@CatWithApple CatWithApple merged commit ef4ad49 into master Mar 28, 2019
@CatWithApple
Copy link
Contributor Author

published [email protected]

@sipayRT sipayRT deleted the report-creation-date branch November 28, 2019 08:09
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