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

[new-ci] Reference for stage 3 temporal #37344

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

Conversation

Josh-Cena
Copy link
Member

@Josh-Cena Josh-Cena commented Dec 23, 2024

#27745

Far from ready for review, and I'll work on this alone for a while

It is now ready for review! It's extremely big but I think it's more worthwhile getting it in as a whole. I`m already leaving the guide to a second PR.

For reviewers (both editorial and technical): I've made a spreadsheet here: https://docs.google.com/spreadsheets/d/1RrRHL0m08R4V4WtuWBbSWr9a1gO4RiIaH42WrPv_Uhs/edit Welcoming anyone on the team for editorial reviews! Technical review from the champion group is greatly appreciated too. It seems we have roughly 4 weeks to make this happen.

@github-actions github-actions bot added Content:JS JavaScript docs size/xl [PR only] >1000 LoC changed labels Dec 23, 2024
Copy link
Contributor

github-actions bot commented Dec 23, 2024

Preview URLs (282 pages)
External URLs (41)

URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON
Title: JSON


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal
Title: Temporal


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/PlainDate
Title: Temporal.PlainDate


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/PlainDate/day
Title: Temporal.PlainDate.prototype.day


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/Instant
Title: Temporal.Instant


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/PlainYearMonth
Title: Temporal.PlainYearMonth


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/ZonedDateTime
Title: Temporal.ZonedDateTime


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/ZonedDateTime/day
Title: Temporal.ZonedDateTime.prototype.day


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/ZonedDateTime/add
Title: Temporal.ZonedDateTime.prototype.add()


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/ZonedDateTime/since
Title: Temporal.ZonedDateTime.prototype.since()


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/PlainDateTime
Title: Temporal.PlainDateTime


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/PlainMonthDay
Title: Temporal.PlainMonthDay


URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/Duration
Title: Temporal.Duration

TRUNCATED!

@Josh-Cena Josh-Cena marked this pull request as ready for review January 3, 2025 09:04
@Josh-Cena Josh-Cena requested a review from a team as a code owner January 3, 2025 09:04
@Josh-Cena Josh-Cena requested review from chrisdavidmills and hamishwillee and removed request for a team January 3, 2025 09:04
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

For Temporal.Now

Copy link
Contributor

@meyerweb meyerweb left a comment

Choose a reason for hiding this comment

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

PlainTime, PlainYearMonth, and ZonedDateTime indexes

Copy link
Contributor

@meyerweb meyerweb left a comment

Choose a reason for hiding this comment

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

Temporal index page

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Giving my approval, so I don't end up blocking this one later on.

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

One comment for Time zone & calendar group

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

One comment for Time components group

Copy link
Contributor

@meyerweb meyerweb left a comment

Choose a reason for hiding this comment

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

Temporal.Instant constructors

Copy link
Contributor

@meyerweb meyerweb left a comment

Choose a reason for hiding this comment

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

Temporal.PlainDate constructors

Copy link
Contributor

@meyerweb meyerweb left a comment

Choose a reason for hiding this comment

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

Temporal.PlainDateTime and Temporal.PlainTime constructors

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Few comments for Arithmetic group

Copy link
Contributor

@meyerweb meyerweb left a comment

Choose a reason for hiding this comment

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

Temporal.PlainMonthDay and Temporal.PlainYearMonth constructors

Copy link
Contributor

@meyerweb meyerweb left a comment

Choose a reason for hiding this comment

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

Temporal.ZonedDateTime constructors

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Only one comment for Calendar: month-related group

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Jan 14, 2025
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Jan 14, 2025
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Couple of comments for Conversion group, nearly there

console.log(zonedDateTime.toString()); // 2021-08-01T08:34:56.123456789-04:00[America/New_York]

const localDateTime = instant.toZonedDateTimeISO(Temporal.Now.timeZoneId());
console.log(zonedDateTime.toString()); // This instant in your timezone
Copy link
Member

Choose a reason for hiding this comment

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

copy paste err?

Suggested change
console.log(zonedDateTime.toString()); // This instant in your timezone
console.log(localDateTime.toString()); // This instant in your timezone


```js
const dtNotExist = Temporal.PlainDateTime.from("2024-03-10T02:05:00");
// This wall-clock time never existed. So we need to choose from 01:05:00-05:00 or 03:05:00-04:00.
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to mention "due to DST changeover" in the comment?

Copy link
Member

Choose a reason for hiding this comment

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

I find the interleaving ambiguous and not-existing examples is a little hard to follow, but leave if you prefer it.

The **`toPlainDateTime()`** method of {{jsxref("Temporal.ZonedDateTime")}} instances returns a new {{jsxref("Temporal.PlainDateTime")}} object representing the date and time portions of this date-time. Only the time zone information is removed.

> [!WARNING]
> After a `Temporal.ZonedDateTime` is converted to `Temporal.PlainDateTime`, it will no longer be aware of its time zone. This means that subsequent operations like arithmetic or with will not adjust for DST and may not yield the same results as equivalent operations with `Temporal.ZonedDateTime`. However, unless you perform those operations across a time zone offset transition, it's impossible to notice the difference. Therefore, be very careful when performing this conversion because subsequent results may look correct most of the time while failing around time zone transitions like when DST starts or ends.
Copy link
Member

@bsmth bsmth Jan 15, 2025

Choose a reason for hiding this comment

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

Minor one if you like:

Suggested change
> After a `Temporal.ZonedDateTime` is converted to `Temporal.PlainDateTime`, it will no longer be aware of its time zone. This means that subsequent operations like arithmetic or with will not adjust for DST and may not yield the same results as equivalent operations with `Temporal.ZonedDateTime`. However, unless you perform those operations across a time zone offset transition, it's impossible to notice the difference. Therefore, be very careful when performing this conversion because subsequent results may look correct most of the time while failing around time zone transitions like when DST starts or ends.
> After a `Temporal.ZonedDateTime` is converted to `Temporal.PlainDateTime`, it's no longer time zone aware. Subsequent operations like arithmetic or `with` operations will not adjust for DST and may not yield the same results as equivalent operations with `Temporal.ZonedDateTime`. However, unless you perform those operations across a time zone offset transition, it's impossible to notice the difference. Be very careful when performing this conversion because subsequent results may be correct most of the time, but incorrect across time zone transitions like when DST starts or ends.

{{JSRef}}

The **`toPlainTime()`** method of {{jsxref("Temporal.ZonedDateTime")}} instances returns a new {{jsxref("Temporal.PlainTime")}} object representing the time portion of this date-time.

Copy link
Member

Choose a reason for hiding this comment

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

Is the warning like on toplaindatetime also useful regarding accuracy of subsequent calculations across DST boundaries, etc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:JS JavaScript docs size/xl [PR only] >1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants