ngx-bootstrap icon indicating copy to clipboard operation
ngx-bootstrap copied to clipboard

Feat time picker display utc

Open juicyarts opened this issue 2 years ago • 11 comments

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.

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

closes #5635

juicyarts avatar Oct 10 '21 11:10 juicyarts

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 https://github.com/valor-software/ngx-bootstrap/issues/4731, right?

daniloff200 avatar Oct 10 '21 11:10 daniloff200

Hey @daniloff200 , this one is related to #5635 , there's another one in my fork that handles offset / #4731 here: https://github.com/juicyarts/ngx-bootstrap/pull/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

juicyarts avatar Oct 10 '21 11:10 juicyarts



Test summary

800 32 14 0


Run details

Project ngx-bootstrap
Status Failed
Commit ac5fe05713 ℹ️
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

cypress[bot] avatar Oct 10 '21 12:10 cypress[bot]

@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 avatar Oct 10 '21 12:10 juicyarts

This one https://github.com/juicyarts/ngx-bootstrap/pull/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..

juicyarts avatar Oct 10 '21 13:10 juicyarts

Codecov Report

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

:exclamation: 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%) :arrow_up:
src/timepicker/timepicker.config.ts 100.00% <100.00%> (ø)
src/carousel/utils.ts 44.44% <0.00%> (-27.78%) :arrow_down:
src/chronos/i18n/it.ts 80.00% <0.00%> (-20.00%) :arrow_down:
src/positioning/utils/getBoundingClientRect.ts 72.22% <0.00%> (-19.45%) :arrow_down:
src/carousel/carousel.component.ts 44.21% <0.00%> (-7.29%) :arrow_down:
src/dropdown/bs-dropdown-container.component.ts 45.71% <0.00%> (-5.72%) :arrow_down:
src/chronos/i18n/th.ts 57.14% <0.00%> (-5.36%) :arrow_down:
... 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.

codecov[bot] avatar Oct 11 '21 11:10 codecov[bot]

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

juicyarts avatar Oct 12 '21 11:10 juicyarts

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

daniloff200 avatar Oct 12 '21 16:10 daniloff200

@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

valorkin avatar Nov 08 '21 11:11 valorkin

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. https://github.com/juicyarts/ngx-bootstrap/pull/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 avatar Nov 14 '21 12:11 juicyarts

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

juicyarts avatar Dec 04 '21 12:12 juicyarts