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

fixed issue edit pattern not applied #1030

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

fixed issue edit pattern not applied #1030

wants to merge 6 commits into from

Conversation

egmox
Copy link
Contributor

@egmox egmox commented Jan 5, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

vdua
vdua requested changes Jan 5, 2024
View reviewed changes
ui.frontend/src/view/FormDatePicker.js Outdated Show resolved Hide resolved
...t/apps/core/fd/components/form/datepicker/v1/datepicker/clientlibs/site/js/datepickerview.js Outdated
@@ -78,10 +79,10 @@
this.widgetObject = new DatePickerWidget(this, this.getWidget(), model);
}
if (this.widgetObject.getValue() !== '') {
this._model.value = this.widgetObject.getValue();
super.updateFormattedDate(this.widgetObject.getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to keep the changes simple here, no new class for now

if (editFormat) {
   this._model.value = parse(...,...);
} else {
   this._model.value = this.widgetObject.getValue()
}

And fix af2-web-runtime to allow setting date objects in value.

rismehta
rismehta requested changes Jan 8, 2024
View reviewed changes
Copy link
Collaborator

@rismehta rismehta left a comment

Choose a reason for hiding this comment

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

check comments

@egmox egmox force-pushed the fixed_date_editPattern branch 3 times, most recently from 197f8ca to 4f4a747 Compare January 9, 2024 12:45
@codecov Codecov
Copy link

codecov bot commented Jan 9, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (479a4d1) 80.40% compared to head (8b4ab7e) 80.40%.

Additional details and impacted files 
@@            Coverage Diff            @@
##                dev    #1030   +/-   ##
=========================================
  Coverage     80.40%   80.40%           
  Complexity      738      738           
=========================================
  Files            91       91           
  Lines          2057     2057           
  Branches        273      273           
=========================================
  Hits           1654     1654           
  Misses          251      251           
  Partials        152      152

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vdua
vdua requested changes Jan 9, 2024
View reviewed changes
Copy link
Collaborator

@vdua vdua left a comment

Choose a reason for hiding this comment

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

Please incorporate the comments and update the PR

rismehta
rismehta reviewed Jan 17, 2024
View reviewed changes
ui.tests/test-module/specs/datepicker/datepicker.runtime.spec.js
@@ -195,7 +197,8 @@ describe("Form Runtime with Date Picker", () => {

it("Test order of the days", () => {
const [datePicker7, datePicker7FieldView] = Object.entries(formContainer._fields)[6];
cy.get(`#${datePicker7}`).find(".cmp-adaptiveform-datepicker__calendar-icon").should("be.visible").click().then(() => {
cy.get(`#${datePicker7}`).find(".cmp-adaptiveform-datepicker__calendar-icon").should("be.visible")
.eq(0).click({force: true}).then(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have you changed the existing test suite ? Isn't this a breaking change ? Ideally with your changes, you only need to add a new test spec. Please remove the changes to the existing test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some tests were failing because all the calendar icons are on top right of the page and overlapping each other.
They were failing after I added test cases and were probably flaky earlier also.

rismehta
rismehta reviewed Jan 17, 2024
View reviewed changes
ui.tests/test-module/specs/datepicker/datepicker.runtime.spec.js
const [datePicker7] = Object.entries(formContainer._fields)[6];

cy.get(`#${datePicker7}`).find(".cmp-adaptiveform-datepicker__calendar-icon").should("be.visible")
.eq(0).click({force: true}).then(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No use of force:true on runtime test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

force true is used because the calendar icons are overlapping each other because the test collaterals do not have css

rismehta
rismehta reviewed Jan 17, 2024
View reviewed changes
...t/apps/core/fd/components/form/datepicker/v1/datepicker/clientlibs/site/js/datepickerview.js
setModel(model) {
super.setModel(model);
if (!this.#noFormats()) {
if (this.widgetObject == null) {
this.widgetObject = new DatePickerWidget(this, this.getWidget(), model);
}
if (this.widgetObject.getValue() !== '') {
this._model.value = this.widgetObject.getValue();
this._model.value = this.getFormattedDate(this.widgetObject.getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, you are basically trying to get the data value which is in ISO format from a formatted value. We should have a function for this in af-core and direcly use it, rather than copying the entire logic of formatters here ? @vdua IIRC, we do have some utility for this ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not yet, but I asked @egmox to create it in runtime. I would suggest making editValue writable property and set this._model.editValue = user entered value
and the setter should parse it accordingly.

rismehta
rismehta requested changes Jan 17, 2024
View reviewed changes
Copy link
Collaborator

@rismehta rismehta left a comment

Choose a reason for hiding this comment

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

check comments

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

@vdua vdua vdua requested changes

@rismehta rismehta rismehta requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@egmox @vdua @rismehta