Skip to content

Add timezone client-hints#303

Closed
rajeshg wants to merge 0 commit intoepicweb-dev:mainfrom
rajeshg:main
Closed

Add timezone client-hints#303
rajeshg wants to merge 0 commit intoepicweb-dev:mainfrom
rajeshg:main

Conversation

@rajeshg
Copy link
Copy Markdown
Contributor

@rajeshg rajeshg commented Jul 12, 2023

Full credits to @kiliman for this work. https://github.com/kiliman/epic-stack-time-zone

Client hints for timezone are a nice add. The approach is similar to the way themes are handled currently. CH-time-zone will be added to the cookies.

Test Plan

Checklist

  • Tests updated
  • Docs updated

Screenshots

Copy link
Copy Markdown
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

I love this. Just a few changes.

Comment on lines +22 to +24
transform(value: string | null) {
return value ?? 'UTC'
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this case, we don't need to transform the value at all and the fallback is already defined. I think we should not require the transform function. So let's remove it here.

Suggested change
transform(value: string | null) {
return value ?? 'UTC'
},

This change may necessitate a small update in the code below.

[name in ClientHintNames]: ReturnType<
(typeof clientHints)[name]['transform']
>
[name in ClientHintNames]: string
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We lose some type-checking here. But couldn't think of a better way to do it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah yeah, we'll need to figure out how to have our cake and eat it too here 😅 If you can't work it out, then that's fine. I'll come back to this later when I find time. Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Couldn't figure out :(

@rajeshg rajeshg requested a review from kentcdodds July 12, 2023 21:19
@rajeshg rajeshg closed this Jul 13, 2023
@kentcdodds
Copy link
Copy Markdown
Member

Why was this PR closed? I'd still like to have this feature eventually and I'd prefer you get git credit for your work :)

I might recommend you make any future PRs from a non-main branch though.

@rajeshg
Copy link
Copy Markdown
Contributor Author

rajeshg commented Jul 13, 2023

My mistake. You are right, I should have done that from a non-main branch. I attempted to copy over the latest changes from epicweb-dev/epic-stack to rajeshg/epic-stack.

@rajeshg
Copy link
Copy Markdown
Contributor Author

rajeshg commented Jul 13, 2023

Couldn't reopen this myself, since I lack the rights to reopen pull requests on this repo. I will submit a separate pull request, when I get a chance.

@kentcdodds
Copy link
Copy Markdown
Member

I can't reopen it either because you created it from main and now your main branch is different. This is another reason using a different branch helps 😅 Thanks!

@rajeshg rajeshg mentioned this pull request Jul 13, 2023
2 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.

3 participants