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.

Sign up for GitHub

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

Jump to bottom

Support datetime component #377

Merged
merged 6 commits into from
Dec 12, 2022
Merged

Support datetime component #377

merged 6 commits into from
Dec 12, 2022

Conversation

Skaiir
Copy link
Contributor

@Skaiir Skaiir commented Oct 28, 2022 •
edited by pinussilvestrus
Loading

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Oct 28, 2022
Skaiir
Skaiir commented Oct 28, 2022
View reviewed changes
packages/form-js/package.json Outdated Show resolved Hide resolved
@pinussilvestrus
Copy link
Contributor

Let's not forget to update the schemaVersion, let's do it already with this PR.

pinussilvestrus
pinussilvestrus suggested changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@pinussilvestrus pinussilvestrus left a comment

Choose a reason for hiding this comment

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

Functionality-wise, I already like it a lot 👍

I also gave a first review code-wise. It's a lot, so I didn't check the datetime conversion magic in full detail but will give it a thorough review in a second step.

packages/form-js-editor/src/features/properties-panel/entries/DateTimeConstraintsEntry.js Show resolved Hide resolved
packages/form-js-editor/src/features/properties-panel/entries/DateTimeEntry.js Outdated Show resolved Hide resolved
packages/form-js-viewer/src/render/components/form-fields/Datetime.js Outdated Show resolved Hide resolved
packages/form-js-viewer/package.json Outdated Show resolved Hide resolved
packages/form-js-viewer/package.json Outdated Show resolved Hide resolved
packages/form-js-viewer/src/render/components/form-fields/Datetime.js Outdated Show resolved Hide resolved
packages/form-js-viewer/src/render/components/form-fields/Datetime.js Outdated Show resolved Hide resolved
packages/form-js-viewer/src/render/components/form-fields/parts/Datepicker.js Outdated Show resolved Hide resolved
packages/form-js-viewer/src/render/components/form-fields/parts/Datepicker.js Outdated Show resolved Hide resolved
packages/form-js-viewer/src/render/components/form-fields/parts/Timepicker.js Outdated Show resolved Hide resolved
@pinussilvestrus
Copy link
Contributor

I rebased the branch on develop to keep it in sync.

@Skaiir
Copy link
Contributor Author

Skaiir commented Nov 28, 2022 •
edited
Loading

Stuff that still needs doing:

  • [bug] datepicker dropdown sometimes closes on clicking anywhere that isn't a day
  • [bug] date field input should be focused after selection to be consistent with time field
  • [bug] C:\dev\proj\camunda\bpmn-io\form-js\packages\form-js-editor\src\features\properties-panel\entries\ValuesSourceUtil doesn't exist
  • Have two configurable labels in datetime subtype
  • Only display 'two fields must be completed' error when neither of the fields aren't focused
  • Rebase and resolve merge conflicts 😨
  • tests
    • schema testing
    • properties panel interaction testing
    • component testing
      • date subtype
      • time subtype
      • datetime subtype
    • serialization logic testing
    • fix broken tests
      • dropdown mouse navigation tests
      • should render above test
      • one broken test in the date subtype
      • couple broken tests in schema related to bug outlined below

Discovered Nov 30th

  • Clear unneeded configs when switching from a date to a time subtype
  • [bug] hour dropdown doesn't seek in datetime subtype when date isn't set
  • Simplify properties panel groupings
  • test config clearing behavior

Discovered December 1st

  • [bug] Setting values manually and then pressing ArrowDown breaks datepicker state
  • [bug] validation doesn't color field red, again

Discovered December 5th

  • [bug] Input data is unexpectedly cleared in tests when passed through the importer?
  • [bug] Data isn't passed from input in time subtype.

@Skaiir Skaiir force-pushed the 340-datetimepicker branch 2 times, most recently from d066db0 to 335a134 Compare December 4, 2022 22:23
pinussilvestrus
pinussilvestrus reviewed Dec 8, 2022
View reviewed changes
packages/form-js-editor/test/spec/FormEditor.spec.js Outdated Show resolved Hide resolved
packages/form-js-playground/test/TestHelper.js Outdated Show resolved Hide resolved
packages/form-js-viewer/package.json Outdated Show resolved Hide resolved
packages/form-js-viewer/src/Form.js Outdated Show resolved Hide resolved
packages/form-js-viewer/test/spec/render/components/form-fields/Datetime.spec.js Outdated Show resolved Hide resolved
packages/form-js/package.json Outdated Show resolved Hide resolved
packages/form-js/test/spec/FormEditor.spec.js Outdated Show resolved Hide resolved
@Skaiir Skaiir mentioned this pull request Dec 8, 2022
Closed
@Skaiir Skaiir marked this pull request as ready for review December 9, 2022 06:43
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 9, 2022
@pinussilvestrus
Copy link
Contributor

pinussilvestrus commented Dec 9, 2022 •
edited
Loading

We will squash and split the commits after the final review.

Update: just did this.

@pinussilvestrus pinussilvestrus mentioned this pull request Dec 9, 2022
Merged
@pinussilvestrus pinussilvestrus changed the title DateTimePicker Draft PR Support datetime component Dec 9, 2022
pinussilvestrus
pinussilvestrus approved these changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@pinussilvestrus pinussilvestrus left a comment

Choose a reason for hiding this comment

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

This looks good from my side now 🎉 Quite an extensive test coverage, I pretty much like 🙂

I'll wait for UX feedback from @RomanKostka @christian-konrad before merging. If you are interested, you can find the datetime schema here.

@Skaiir Skaiir merged commit 79888b5 into develop Dec 12, 2022
@Skaiir Skaiir deleted the 340-datetimepicker branch December 12, 2022 09:31
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 12, 2022
@philippfromme philippfromme mentioned this pull request Jan 5, 2023
Closed
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinussilvestrus pinussilvestrus pinussilvestrus approved these changes

Assignees

@Skaiir Skaiir

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement new DateTimePicker component
2 participants
@Skaiir @pinussilvestrus