Skip to content

feat: Add Eastern and Eastern Pacific time zones #16880#16974

Merged
emrysal merged 8 commits intocalcom:mainfrom
ShreyTanna29:feature/addEasternTimezones#16880
Oct 10, 2024
Merged

feat: Add Eastern and Eastern Pacific time zones #16880#16974
emrysal merged 8 commits intocalcom:mainfrom
ShreyTanna29:feature/addEasternTimezones#16880

Conversation

@ShreyTanna29
Copy link
Copy Markdown
Contributor

@ShreyTanna29 ShreyTanna29 commented Oct 7, 2024

What does this PR do?

Added Eastern Time - US Canada, Pacific Time , etc
In total 15 time zones are added.

Mandatory Tasks (DO NOT REMOVE)

  • [ YES] I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • [N/A ] I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • [ YES] I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

visit any page which has time zones options in it and you can see time zones like Eastern Time - US Canada, Central TIme, Mountain Time, Indian Standard TIme, etc.

  • Are there environment variables that should be set? NO
  • What are the minimal test data to have? No specific test data required
  • What is expected (happy path) to have (input and output)? Getting newly added time zones in time zones dropdowns
  • Any other important info that could help to test that PR. NO

@vercel
Copy link
Copy Markdown

vercel Bot commented Oct 7, 2024

@ShreyTanna29 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app Bot added the community Created by Linear-GitHub Sync label Oct 7, 2024
@graphite-app graphite-app Bot requested a review from a team October 7, 2024 17:12
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Oct 7, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions Bot added enterprise area: enterprise, audit log, organisation, SAML, SSO Medium priority Created by Linear-GitHub Sync ✨ feature New feature or request labels Oct 7, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 7, 2024

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "feat : Add Eastern and Eastern Pacific time zones  #16880". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@dosubot dosubot Bot added this to the v4.6 milestone Oct 7, 2024
@graphite-app
Copy link
Copy Markdown

graphite-app Bot commented Oct 7, 2024

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (10/07/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (10/07/24)

1 label was added to this PR based on Keith Williams's automation.

"Add ready-for-e2e label" took an action on this PR • (10/10/24)

1 label was added to this PR based on Keith Williams's automation.

@ShreyTanna29 ShreyTanna29 changed the title feature : Add Eastern and Eastern Pacific time zones #16880 feat : Add Eastern and Eastern Pacific time zones #16880 Oct 7, 2024
@ShreyTanna29 ShreyTanna29 changed the title feat : Add Eastern and Eastern Pacific time zones #16880 feat: Add Eastern and Eastern Pacific time zones #16880 Oct 7, 2024
@Praashh
Copy link
Copy Markdown
Contributor

Praashh commented Oct 8, 2024

Hey @ShreyTanna29 Thanks for contributing, can you please sign CLA.

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

Hey @ShreyTanna29 Thanks for contributing, can you please sign CLA.

Hello, I have already signed the CLA, please check again.

@Praashh
Copy link
Copy Markdown
Contributor

Praashh commented Oct 8, 2024

getTimezone should return the appropriate time zones for a given city name. @ShreyTanna29 could you please check this?

@Praashh
Copy link
Copy Markdown
Contributor

Praashh commented Oct 8, 2024

Hey @ShreyTanna29 Thanks for contributing, can you please sign CLA.

Hello, I have already signed the CLA, please check again.

Also, test PR Update / Tests / Unit (pull_request_target) isn't working as expected.

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

ShreyTanna29 commented Oct 9, 2024

getTimezone should return the appropriate time zones for a given city name. @ShreyTanna29 could you please check this?

@Praashh This was due to matching the " timezones recieved in addCitiesToDropDown" with the inlineSnapshot provided in getTimezone.test.ts, which was throwing error whenever a new timezone appeared, so I added new timezones in the snapshot , please check the latest commits, this issue is now solved. please let me know if any changes are neede further.

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

Hey @ShreyTanna29 Thanks for contributing, can you please sign CLA.

Hello, I have already signed the CLA, please check again.

Also, test PR Update / Tests / Unit (pull_request_target) isn't working as expected.

I checked 'PR Update / Tests / Unit (pull_request_target)` I couldn't find any issues over there. Please refer the attached screen shots, Please provide more details on it.
pr-update-test--unit
tests-units ScreeenShot

@Praashh
Copy link
Copy Markdown
Contributor

Praashh commented Oct 9, 2024

Hey @ShreyTanna29 Thanks for contributing, can you please sign CLA.

Hello, I have already signed the CLA, please check again.

Also, test PR Update / Tests / Unit (pull_request_target) isn't working as expected.

I checked 'PR Update / Tests / Unit (pull_request_target)` I couldn't find any issues over there. Please refer the attached screen shots, Please provide more details on it. pr-update-test--unit tests-units ScreeenShot

yes, it was there and after fixing the timezones issue, it also got fixed.

emrysal
emrysal previously approved these changes Oct 10, 2024
Copy link
Copy Markdown
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 10, 2024

E2E results are ready!

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

ShreyTanna29 commented Oct 10, 2024

@Praashh @emrysal , it seems this pr isn't getting auto merge as it fails some tests that has nothing to do with changes I made in the code and also I dont know how to fix them, please help.

@Praashh
Copy link
Copy Markdown
Contributor

Praashh commented Oct 10, 2024

@Praashh @emrysal , it seems this pr isn't getting auto merge as it fails some tests that has nothing to do with changes I made in the code and also I dont know how to fix them, please help.

Hey @ShreyTanna29, don't worry about those tests, please make changes I suggested to maintain good practices and directory structure so I can approve it, although your code is working fine.

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

ShreyTanna29 commented Oct 10, 2024

@Praashh @emrysal , it seems this pr isn't getting auto merge as it fails some tests that has nothing to do with changes I made in the code and also I dont know how to fix them, please help.

Hey @ShreyTanna29, don't worry about those tests, please make changes I suggested to maintain good practices and directory structure so I can approve it, although your code is working fine.

thank you ,and please guide me if you think that anything in this code can be improved , also I have followed and applied your previous suggestions and would love to change further anything required in this code.

@Praashh
Copy link
Copy Markdown
Contributor

Praashh commented Oct 10, 2024

@Praashh @emrysal , it seems this pr isn't getting auto merge as it fails some tests that has nothing to do with changes I made in the code and also I dont know how to fix them, please help.

Hey @ShreyTanna29, don't worry about those tests, please make changes I suggested to maintain good practices and directory structure so I can approve it, although your code is working fine.

thank you ,and please guide me if you think that anything in this code can be improved , also I have followed and applied your previous suggestions and would love to change further anything required in this code.

Hey! I have already suggested you, please add those changes.

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

@Praashh I have applied those suggestions, you asked to check "getTimezone should return the appropriate time zones for a given city name.", I have resolved this and you also asked to check this "PR Update / Tests / Unit (pull_request_target) " which is also resolved. please check the pr commits.

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

ShreyTanna29 commented Oct 10, 2024

@Praashh @emrysal , it seems this pr isn't getting auto merge as it fails some tests that has nothing to do with changes I made in the code and also I dont know how to fix them, please help.

Hey @ShreyTanna29, don't worry about those tests, please make changes I suggested to maintain good practices and directory structure so I can approve it, although your code is working fine.

sorry but what directory structure and good practices did you suggested, cause I am not able to see any comment regarding that.

@Praashh
Copy link
Copy Markdown
Contributor

Praashh commented Oct 10, 2024

@Praashh @emrysal , it seems this pr isn't getting auto merge as it fails some tests that has nothing to do with changes I made in the code and also I dont know how to fix them, please help.

Hey @ShreyTanna29, don't worry about those tests, please make changes I suggested to maintain good practices and directory structure so I can approve it, although your code is working fine.

sorry but what directory structure and good practices did you suggested, cause I cant see any comment regarding that.

please check this, https://github.com/calcom/cal.com/pull/16974/files/f5ece1f4dabaf68df5c44a6d8245ca6b7bb8f3f9#diff-22aa63d18fd13b882f1abed60dae3b1560e2b517602e8a6a0babad035a77fe58

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

@Praashh @emrysal , it seems this pr isn't getting auto merge as it fails some tests that has nothing to do with changes I made in the code and also I dont know how to fix them, please help.

Hey @ShreyTanna29, don't worry about those tests, please make changes I suggested to maintain good practices and directory structure so I can approve it, although your code is working fine.

sorry but what directory structure and good practices did you suggested, cause I cant see any comment regarding that.

please check this, https://github.com/calcom/cal.com/pull/16974/files/f5ece1f4dabaf68df5c44a6d8245ca6b7bb8f3f9#diff-22aa63d18fd13b882f1abed60dae3b1560e2b517602e8a6a0babad035a77fe58

sorry but i cant see any changes requested on the files , no conversation on the file page, no comments on the file, no inline comments or no commits from your side, have I missed any section to look for your suggestions, exactly where have you posted your suggestions on the file page, please guide me.

Copy link
Copy Markdown
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

Fixed availability test

@emrysal emrysal enabled auto-merge (squash) October 10, 2024 16:06
@emrysal emrysal merged commit b1cd3e9 into calcom:main Oct 10, 2024
@ShreyTanna29 ShreyTanna29 deleted the feature/addEasternTimezones#16880 branch October 10, 2024 17:01
@PeerRich
Copy link
Copy Markdown
Member

regression: cities are not showing anymore

image

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

regression: cities are not showing anymore

image

Looking into it

@emrysal
Copy link
Copy Markdown
Contributor

emrysal commented Oct 11, 2024

@PeerRich @ShreyTanna29 - I'll add a test to cover this in the follow up PR to fix

@ShreyTanna29
Copy link
Copy Markdown
Contributor Author

regression: cities are not showing anymore

image

I have solved the issue, please check Pull request #17064 , this was due to my mistake, I had by mistake added timezones as cities in the dropdown, I have fixed this everywhere, also I have improved naming of my new timezones in additionalTimezones.json, I have followed the cal.com prewritten naming convections, e.g. instead of identifier I named it as city, ect. Sorry for the mistake. Please check it out and let me know if any improvements needs to be done further. @PeerRich @emrysal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community Created by Linear-GitHub Sync enterprise area: enterprise, audit log, organisation, SAML, SSO ✨ feature New feature or request Medium priority Created by Linear-GitHub Sync ready-for-e2e

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-4434] feature request: Add Eastern Time -US & Canada, Pacific Easter etc. in timezone picker

5 participants