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

Feat : custom timezone spec (#723) #721

Merged
merged 35 commits into from
Dec 23, 2020
Merged

Conversation

jungeun-cho
Copy link
Contributor

@jungeun-cho jungeun-cho commented Nov 22, 2020

Please check if the PR fulfills these requirements

  • It's submitted to right branch according to our branching model
  • It's right issue type on title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has description for the breaking change

Description

  • If the system local setting uses a time zone that includes DST, the schedule should always be rendered the same, regardless of whether the time is summer or standard time at the time the calendar instance is created.

  • The custom timezone spec has been changed. (feat Custom time zone designation option specification definition #723 )


Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

@jungeun-cho jungeun-cho changed the title Feat : custom timezone spec Feat : custom timezone spec (#723) Nov 22, 2020
@jungeun-cho
Copy link
Contributor Author

ok to test

Copy link
Contributor

@dongsik-yoo dongsik-yoo left a comment

Choose a reason for hiding this comment

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

진행한 데까지는 남겨놓고 이어서할게요.

src/js/common/intlUtil.js Outdated Show resolved Hide resolved
src/js/common/intlUtil.js Outdated Show resolved Hide resolved
src/js/common/intlUtil.js Outdated Show resolved Hide resolved
src/js/common/intlUtil.js Outdated Show resolved Hide resolved
src/js/common/intlUtil.js Outdated Show resolved Hide resolved
test/app/common/timezone.spec.js Outdated Show resolved Hide resolved
src/js/common/timezone.js Outdated Show resolved Hide resolved

/**
* Set primary timezone code
* @param {string} timezoneCode - timezone code (such as 'Asia/Seoul', 'America/New_York')
Copy link
Contributor

Choose a reason for hiding this comment

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

"IANA time zone names" 이라고 명시해 주면 좋을 것 같아요.

Copy link
Contributor Author

@jungeun-cho jungeun-cho Dec 9, 2020

Choose a reason for hiding this comment

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

timezone name (time zone names of the IANA time zone database, such as 'Asia/Seoul', 'America/New_York')

이렇게 설명하는건어떻할까요?

* @param {Timezone} timezoneObj - {@link Timezone}
*/
function setPrimaryTimezoneByOption(timezoneObj) {
var timezoneCode = timezoneObj.timezone;
Copy link
Contributor

Choose a reason for hiding this comment

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

timezone, timeZone의 네이밍이 통일되어 있지 않습니다. 적어도 인터페이스에서는 맞추어야 되겠습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

인터페이스 확인 부탁드립니다.

    timezone: {
      zones: [
        {
          timezoneName: 'Asia/Seoul',
          displayLabel: 'GMT+09:00',
          tooltip: 'Seoul'
        },
        {
          timezoneName: 'America/New_York',
          displayLabel: 'GMT-05:00',
          tooltip: 'New York',
        }
      ],
      offsetCalculator: function(timezone, timestamp){
        // e.g. +09:00 => -540, -04:00 => 240
        return moment.tz.zone(timezone).utcOffset(timestamp);
      },
    }

Copy link
Contributor

@dongsik-yoo dongsik-yoo left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다. 고생하십니다!

src/js/common/timezone.js Outdated Show resolved Hide resolved
src/js/common/timezone.js Outdated Show resolved Hide resolved
src/js/common/timezone.js Outdated Show resolved Hide resolved
src/js/common/timezone.js Outdated Show resolved Hide resolved
src/js/common/timezone.js Outdated Show resolved Hide resolved
src/js/model/schedule.js Outdated Show resolved Hide resolved
src/js/view/popup/scheduleDetailPopup.js Outdated Show resolved Hide resolved
src/js/view/week/time.js Show resolved Hide resolved
test/app/common/timezone.spec.js Outdated Show resolved Hide resolved
test/app/common/timezone.spec.js Outdated Show resolved Hide resolved
@@ -49,7 +46,7 @@ var differentOffset = {
* @private
*/
function getTimezoneOffset(timestamp) {
timestamp = timestamp || Date.now();
timestamp = !util.isUndefined(timestamp) ? timestamp : Date.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 이거 빼먹을 뻔했네요! 멋져부러!

@dongsik-yoo
Copy link
Contributor

기존의 timezones가 들어오는 경우도 스크립트 에러나지 않고 timezone.zones쪽으로 타도록 처리되어야 할 것 같은데, 코드에서 못봤네요. timezones 옵션을 처리할 수 있어야 메이저 버전이 올라가지 않을 수 있습니다. 확인 부탁 드려요.

@jungeun-cho
Copy link
Contributor Author

기존의 timezones가 들어오는 경우도 스크립트 에러나지 않고 timezone.zones쪽으로 타도록 처리되어야 할 것 같은데, 코드에서 못봤네요. timezones 옵션을 처리할 수 있어야 메이저 버전이 올라가지 않을 수 있습니다. 확인 부탁 드려요.

네, 하위버전 호환가능하도록 기존에 사용된 timezones옵션 연동하도록 하겠습니다.:)

Copy link
Contributor

@dongsik-yoo dongsik-yoo left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다!

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.

None yet

2 participants