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 time picker display utc #6322

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

juicyarts
Copy link

@juicyarts juicyarts commented Oct 10, 2021

PR Checklist

Before creating new PR, please take a look at checklist below to make sure that you've done everything that needs to be done before we can merge it.

  • read and followed the CONTRIBUTING.md guide.
  • built and tested the changes locally.
  • added/updated tests.
  • added/updated API documentation.
  • added/updated demos.

closes #5635

@daniloff200
Copy link
Contributor

hey @juicyarts !
Good to see u again! Looks like, you've managed to move your work to a new PR
I assume, it's related to #4731, right?

@juicyarts
Copy link
Author

Hey @daniloff200 , this one is related to #5635 , there's another one in my fork that handles offset / #4731 here: juicyarts#1. It made more sense to align the interface with the datepicker for the offsetTarget and use useUtc for consistency, and these two properties (useUtc, offset) can live and work by themselves

@cypress
Copy link

cypress bot commented Oct 10, 2021



Test summary

800 32 14 0


Run details

Project ngx-bootstrap
Status Failed
Commit ac5fe05 ℹ️
Started Jan 17, 2022 1:08 PM
Ended Jan 17, 2022 1:44 PM
Duration 36:20 💡
OS Linux Ubuntu - 20.04
Browser Electron 93

View run in Cypress Dashboard ➡️


Failures

typeahead_page_spec.ts Failed
1 Typeahead demo page testing suite > On blur > clicking anywhere outside auto-fills "Option on blur" with the first option from the matches list
datepicker/locales_spec.ts Failed
1 Datepicker demo testing suite: Locales > Change Locale Datepicker > when user chose locale es-pr for "Datepicker", container shown in appropriate language
2 Datepicker demo testing suite: Locales > Change Locale DateRangepicker > when user chose locale es-pr for "Daterangepicker", container shown in this language
modals_service_page_spec.ts Failed
1 Modals demo page testing suite: Service examples > Nested modals > when user clicks on the button "Open second modal" then the second modal with title "Second modal" is opened, "Close self" is present
2 Modals demo page testing suite: Service examples > Events > when user closes modal by click on the cross then should be messages "onHide event has been fired" and "onHidden event has been fired"
3 Modals demo page testing suite: Service examples > Events > when user closes modal by pressing ESC button then modal is closed and should be messages "onHide event has been fired" and "onHidden event has been fired"
4 Modals demo page testing suite: Service examples > Confirm Window > when user clicks on "Open modal" button then modal is opened, it contains two buttons: "Yes" and "No"
5 Modals demo page testing suite: Service examples > Custom css class > when user clicks on the cross button then the modal is closed
6 Modals demo page testing suite: Service examples > Animation option > when user clicks on the cross button then the modal is closed
7 Modals demo page testing suite: Service examples > Esc closing option > when user clicks on "Open modal" button then modal is opened. when user closes modal by click ESC button then modal stays opened
This comment includes only the first 10 test failures. See all 32 failures in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@juicyarts
Copy link
Author

juicyarts commented Oct 10, 2021

@daniloff200 i have a hard time running the whole unit test suite.. see the logs in the observations below.
The failing e2e tests don't seem to be related to my changes, could this be related to cypress flakiness?

Some observations/issues i documented while working on the pr

test runner inconsistencies

When running npx nx run timepicker:test locally these tests are run:

 PASS   timepicker  src/timepicker/testing/timepicker-controls.util.spec.ts
 PASS   timepicker  src/timepicker/testing/timepicker.component.spec.ts (7.005 s)

When running in ci pipeline:

PASS timepicker src/timepicker/testing/timepicker.component.spec.ts (11.499 s)
PASS timepicker src/timepicker/testing/timepicker-controls.util.spec.ts
FAIL timepicker src/timepicker/testing/timepicker.utils.spec.ts

somehow locally the filter is missing the timepicker.utils.spec file.

EDIT: my bad, seems like the timepicker.utils.spec are all disabled using xdescribe and the comment OMG :D

Datepicker test suites have issues that cause the runner to be stuck

This behaviour can be reproduces in the development branch, so does not seem to be related to my chnages.

 PASS   datepicker  src/datepicker/testing/bs-datepicker-navigation-view.spec.ts
(node:59007) UnhandledPromiseRejectionWarning: TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Zone'
    |     property '_zoneDelegate' -> object with constructor 'ZoneDelegate'
    --- property 'zone' closes the circle
    at stringify (<anonymous>)
    at writeChannelMessage (internal/child_process/serialization.js:117:20)
    at process.target._send (internal/child_process.js:805:17)
    at process.target.send (internal/child_process.js:703:19)
    at reportSuccess (/Users/yildiz/repos/huess/lib/ngx-bootstrap-ja/node_modules/jest-worker/build/workers/processChild.js:67:11)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:59007) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:59007) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:59009) UnhandledPromiseRejectionWarning: TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Zone'
    |     property '_zoneDelegate' -> object with constructor 'ZoneDelegate'
    --- property 'zone' closes the circle
    at stringify (<anonymous>)
    at writeChannelMessage (internal/child_process/serialization.js:117:20)
    at process.target._send (internal/child_process.js:805:17)
    at process.target.send (internal/child_process.js:703:19)
    at reportSuccess (/Users/yildiz/repos/huess/lib/ngx-bootstrap-ja/node_modules/jest-worker/build/workers/processChild.js:67:11)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:59009) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:59009) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

  ●  Cannot log after tests are done. Did you forget to wait for something async in your test?
    Attempted to log "Unhandled Promise rejection: Failed to load timepicker.component.html ; Zone: ProxyZone ; Task: Promise.then ; Value: Failed to load timepicker.component.html undefined".

      at console.error (../../node_modules/@jest/console/build/BufferedConsole.js:161:10)
      at Object.api.onUnhandledError (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:1104:29)
      at handleUnhandledRejection (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:1132:17)
      at _loop_2 (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:1123:21)
      at Object.api.microtaskDrainDone (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:1127:17)
      at drainMicroTaskQueue (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:629:22)
      at ZoneTask.Object.<anonymous>.ZoneTask.invokeTask [as invoke] (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:522:25)
      at invokeTask (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:1658:18)
      at XMLHttpRequest.globalZoneAwareCallback (../../node_modules/zone.js/bundles/zone-testing-bundle.umd.js:1684:21)
      at XMLHttpRequest.callTheUserObjectsOperation (../../node_modules/jsdom/lib/jsdom/living/generated/EventListener.js:26:30)

@juicyarts
Copy link
Author

This one juicyarts#1 builds and tests without any issues.. Since its base is this branch i guess the e2e tests in the last run of this branch were just flaky..

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #6322 (ac2ed8f) into development (58a6e00) will increase coverage by 1.22%.
The diff coverage is 100.00%.

❗ Current head ac2ed8f differs from pull request most recent head 6343ebe. Consider uploading reports for the commit 6343ebe to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6322      +/-   ##
===============================================
+ Coverage        75.84%   77.07%   +1.22%     
===============================================
  Files              315      312       -3     
  Lines            10642    10868     +226     
  Branches          2935     2612     -323     
===============================================
+ Hits              8071     8376     +305     
+ Misses            2570     2475      -95     
- Partials             1       17      +16     
Impacted Files Coverage Δ
apps/ngx-bootstrap-docs/src/ng-api-doc.ts 100.00% <ø> (ø)
src/chronos/public_api.ts 100.00% <100.00%> (ø)
src/timepicker/timepicker.component.ts 92.74% <100.00%> (+5.04%) ⬆️
src/timepicker/timepicker.config.ts 100.00% <100.00%> (ø)
src/carousel/utils.ts 44.44% <0.00%> (-27.78%) ⬇️
src/chronos/i18n/it.ts 80.00% <0.00%> (-20.00%) ⬇️
src/positioning/utils/getBoundingClientRect.ts 72.22% <0.00%> (-19.45%) ⬇️
src/carousel/carousel.component.ts 44.21% <0.00%> (-7.29%) ⬇️
src/dropdown/bs-dropdown-container.component.ts 45.71% <0.00%> (-5.72%) ⬇️
src/chronos/i18n/th.ts 57.14% <0.00%> (-5.36%) ⬇️
... and 136 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58a6e00...6343ebe. Read the comment docs.

@juicyarts
Copy link
Author

So is there something missing or do you want me to change something? Do i need to assign someone here? How can we ensure this PR does not end up like the last one..? :) @daniloff200

@daniloff200
Copy link
Contributor

Well, looks good to me, @juicyarts
Gonna check that tomorrow, how it works

daniloff200
daniloff200 previously approved these changes Oct 12, 2021
@valorkin
Copy link
Member

valorkin commented Nov 8, 2021

@juicyarts hello, thank you so much for your work and help
please convert useUTC from input to configuration option
and I will be glad to merge it

@juicyarts
Copy link
Author

Hey @valorkin, i changed it but am not too sure if this was what you requested. Have a look and let me know if somethings missing. juicyarts#1 is waiting for this one to be merged, i would adopt the approach of only exposing these options in the config too if thats how you want it.

@juicyarts
Copy link
Author

Hey guys. Don't want to be annoying.. Is there something i can still do here to ? @valorkin & @daniloff200 ?

@valorkin valorkin temporarily deployed to ngx-bootstra-feat-time--g6zmli January 17, 2022 10:27 Inactive
@valorkin valorkin temporarily deployed to ngx-bootstra-feat-time--g6zmli January 17, 2022 13:01 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add UTC timezone support to timepicker
3 participants