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

Fixes #32045 - Add start-end date pickers to job wizard #567

Merged
merged 1 commit into from Aug 26, 2021
Merged

Fixes #32045 - Add start-end date pickers to job wizard #567

merged 1 commit into from Aug 26, 2021

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Mar 9, 2021

Updates vendor js to 8.4.0 in order to use datepicker and timepicker.
Depends on #558

@MariaAga MariaAga changed the title Add schedule job wizard datepicker Fixes #32045 - Add start-end date pickers to job wizard Mar 9, 2021
@theforeman-bot
Copy link
Member

@MariaAga, this pull request is currently not mergeable. Please rebase against the master branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream master

This message was auto-generated by Foreman's prprocessor

@MariaAga MariaAga marked this pull request as draft March 9, 2021 13:45
@adamruzicka
Copy link
Contributor

Vendor 8. is >=2.5 only, correct?

@MariaAga
Copy link
Member Author

MariaAga commented Mar 9, 2021

Yes

@MariaAga MariaAga force-pushed the add-schedule-job-wizard-datepicker branch from 31ee864 to b8208a0 Compare June 14, 2021 07:53
@MariaAga MariaAga mentioned this pull request Jun 17, 2021
Merged
@MariaAga MariaAga force-pushed the add-schedule-job-wizard-datepicker branch 2 times, most recently from 6565674 to 97ed9a2 Compare June 18, 2021 07:22
@MariaAga
Copy link
Member Author

js tests are failing because the tests here are using a newer version of pf where they use time instead of defaulttime

@MariaAga MariaAga force-pushed the add-schedule-job-wizard-datepicker branch 2 times, most recently from aa86599 to b04f2cf Compare July 6, 2021 14:43
MariaAga
MariaAga commented Jul 6, 2021
View reviewed changes
webpack/JobWizard/steps/form/DateTimePicker.js
<DatePicker
value={formattedDate}
placeholder="yyyy/mm/dd"
onChange={debounce(onDateChange, 1000, {
Copy link
Member Author

Choose a reason for hiding this comment

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

Any opinions on the wait amount before formatting the users input?

@MariaAga MariaAga force-pushed the add-schedule-job-wizard-datepicker branch 3 times, most recently from 5d9f813 to 9fc3597 Compare July 20, 2021 10:02
@MariaAga MariaAga marked this pull request as ready for review July 20, 2021 10:07
@MariaAga
Copy link
Member Author

@LaViro This is ready for a review 🥳

@adamruzicka
Copy link
Contributor

Needs a rebase

@adamruzicka
Copy link
Contributor

The date and time inputs do not seem to have the same height.

image

@MariaAga
Copy link
Member Author

That might be a pf4 version issue as the time picker was made to be larger on the newer version

@MariaAga MariaAga force-pushed the add-schedule-job-wizard-datepicker branch from 9fc3597 to e069c3a Compare July 21, 2021 12:33
@adamruzicka
Copy link
Contributor

The calendar "escapes" the form, which is what I'd expect to happen
image

However time select is constrainted within the form, which leads to two vertical scroll bars appearing on page. I'd say it should either expand the form vertically or even better, display over other elements on the page
image

@MariaAga MariaAga force-pushed the add-schedule-job-wizard-datepicker branch from e069c3a to 90cb33c Compare July 27, 2021 15:51
@MariaAga
Copy link
Member Author

Added menuAppendTo to the timepicker so the menu will be displayed over other elements

@adamruzicka
Copy link
Contributor

Looks good to me. @LaViro could you take a look?

Ron-Lavi
Ron-Lavi reviewed Aug 24, 2021
View reviewed changes
Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Thanks @MariaAga ! can you also rebase the PR?

package.json Outdated
"@theforeman/stories": "^4.14.0",
"@theforeman/test": "^4.14.0",
"@theforeman/vendor-dev": "^4.14.0",
"@theforeman/builder": "^8.7.0",
Copy link
Member

Choose a reason for hiding this comment

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

Let's split this into another PR? current version is 8.10.0

Copy link
Member Author

Choose a reason for hiding this comment

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

#646

webpack/JobWizard/JobWizard.js Outdated Show resolved Hide resolved
webpack/JobWizard/steps/Schedule/__tests__/Schedule.test.js
expect(endsDateField.disabled).toBeTruthy();
expect(endsTimeField.disabled).toBeTruthy();
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Great test!

@MariaAga MariaAga force-pushed the add-schedule-job-wizard-datepicker branch from 90cb33c to 5800f14 Compare August 25, 2021 10:29
@MariaAga
Copy link
Member Author

Rebased and will now need #646

@adamruzicka adamruzicka merged commit 56af4f2 into theforeman:master Aug 26, 2021
@adamruzicka
Copy link
Contributor

Thank you @MariaAga & @LaViro !

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

@Ron-Lavi Ron-Lavi Ron-Lavi left review comments

Assignees
No one assigned
Labels
Needs re-review Needs testing
Projects
None yet
Milestone
No milestone
4 participants
@MariaAga @theforeman-bot @adamruzicka @Ron-Lavi