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

EZP-29948: Improved dates select on the search content site #1261

Conversation

lucasOsti
Copy link
Contributor

Question Answer
Tickets https://jira.ez.no/browse/EZP-29948
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

michal-myszka
michal-myszka previously approved these changes Feb 26, 2020
src/bundle/Controller/SearchController.php Outdated Show resolved Hide resolved
Copy link
Contributor

@webhdx webhdx left a comment

Choose a reason for hiding this comment

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

Why exactly do you need to add anything to SearchData? It looks like both lastModified and created properties are arrays containing start_date and end_date so you should use those instead of creating new properties. Please map value from your new input type to existing start_date and end_date fields.

Also you need to use proper form types in PHP instead of packing everything to the general form template.

@webhdx webhdx requested a review from alongosz February 26, 2020 13:58
</div>
</div>
</div>

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

},
formatDate: (date) => eZ.helpers.timezone.formatShortDateTime(date, null),
};

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

return;
doc.querySelector(dateRange.dataset.periodSelector).value = `P0Y0M${days}D`;
doc.querySelector(dateRange.dataset.endSelector).value = endDate;
applyBtn.removeAttribute('disabled', false);
Copy link
Member

Choose a reason for hiding this comment

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

You should use toggleDisabledStateOnApplyBtn not making it manually.

if (sourceInput.value) {
defaultDate = new Date(sourceInput.value * 1000);
}
const parseDateUTC = (dateString) => {
Copy link
Member

Choose a reason for hiding this comment

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

this is more like getUnixTimestapUTC, also I think it's not dateString it is date object.

if (selectedDates.length === 2) {
const startDate = parseDateUTC(selectedDates[0]);
const endDate = parseDateUTC(selectedDates[1]);
const days = (endDate - startDate) / 86400;
Copy link
Member

Choose a reason for hiding this comment

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

const secondsInDay = 60 * 60 * 24; was very good way to show what the number is.

@@ -97,6 +98,24 @@
</svg>
<ul class="ez-filters__user-list ez-filters__user-list--hidden"></ul>
</div>
<div class="ez-date-range ez-date-range--select-modified-range {% if form.last_modified_select.vars.data == "custom_range" %}ez-date-range--visible{% endif %}" data-period-selector="#search_last_modified_date_interval" data-end-selector="#search_last_modified_end_date">
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be inside the div ez-filters__item ez-filters__item--modified because it's related to this select.

Also, I would not introduce a new component so this should be named ez-filters__range-wrapper

<div class="ez-date-range ez-date-range--select-modified-range {% if form.last_modified_select.vars.data == "custom_range" %}ez-date-range--visible{% endif %}" data-period-selector="#search_last_modified_date_interval" data-end-selector="#search_last_modified_end_date">
<input
type="text"
class="form-control ez-date-select"
Copy link
Member

Choose a reason for hiding this comment

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

ez-filters__range-select

data-end="{{ formData.lastModified.end_date is defined ? formData.lastModified.end_date|date("Y-m-d") : '' }}"
/>
</div>
<div class="ez-date-range ez-date-range--select-created-range {% if form.created_select.vars.data == "custom_range" %}ez-date-range--visible{% endif %}" data-period-selector="#search_created_date_interval" data-end-selector="#search_created_end_date">
Copy link
Member

Choose a reason for hiding this comment

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

This line is very long, can you make it multiline?

} else {
doc.querySelector(dateRange.dataset.periodSelector).value = '';
doc.querySelector(dateRange.dataset.endSelector).value = 0;
applyBtn.setAttribute('disabled', true);
Copy link
Member

Choose a reason for hiding this comment

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

You can't do that here.

applyBtn.removeAttribute('disabled', false);
} else {
doc.querySelector(dateRange.dataset.periodSelector).value = '';
doc.querySelector(dateRange.dataset.endSelector).value = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you have to do that if this should be set by the toggleDatesSelectVisibility function?

@lucasOsti lucasOsti requested a review from dew326 February 27, 2020 13:50
const sourceInput = doc.querySelector(field.dataset.targetSelector);
const flatPickrInput = field;
let defaultDate;
const getUnixTimestapUTC = (dateObject) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think I did a typo getUnixTimestampUTC, sorry

isCreatedSelected ||
isCreatorSelected ||
isSubtreeSelected) &&
isLastModifiedRangeSelected &&
Copy link
Member

Choose a reason for hiding this comment

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

I think it shouldn't be && it should be enabled if isLastModifiedRangeSelected or isCreatedRangeSelected is filled.

const contentTypeOption = contentTypeSelect.querySelector('option');
const isContentTypeSelected = contentTypeOption.innerHTML !== contentTypeOption.dataset.default;
const isSectionSelected = sectionSelect ? !!sectionSelect.value : false;
const isModifiedSelected = !!lastModifiedSelect.value;
const isCreatedSelected = !!lastCreatedSelect.value;
const isCreatorSelected = !!searchCreatorInput.value;
const isSubtreeSelected = !!subtreeInput.value.trim().length;

if (lastModifiedSelect.value === 'custom_range') {
const flatPickrInstance = lastModifiedDateRange._flatpickr;
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to rely on the flatpickr instance? Cannot we just check if periodSelector and endSelector have values? I think we could replace the isModifiedSelected and isCreatedSelected with this check.

@lucasOsti lucasOsti requested a review from dew326 February 28, 2020 09:23
@lucasOsti lucasOsti requested a review from webhdx February 28, 2020 10:33
@lucasOsti lucasOsti requested review from webhdx and removed request for webhdx February 28, 2020 10:33
@lucasOsti lucasOsti requested a review from dew326 February 28, 2020 12:13
@lucasOsti lucasOsti force-pushed the EZP-29948-Add-date-picker-when-selected-custome-range branch from 2515fa4 to 8667dbb Compare March 3, 2020 08:56
Copy link

@katarzynazawada katarzynazawada left a comment

Choose a reason for hiding this comment

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

As discussed with Supriya, date picker should display only the date not date&time.
Screenshot 2020-03-03 at 10 22 49

locale: {
rangeSeparator: ' - ',
},
formatDate: (date) => eZ.helpers.timezone.formatShortDateTime(date, null),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
formatDate: (date) => eZ.helpers.timezone.formatShortDateTime(date, null),
formatDate: (date) => eZ.helpers.timezone.formatShortDateTime(date, null, eZ.adminUiConfig.dateFormat.shortDate),

To achieve what @katarzynazawada meant we have to add the dateFormat to the formatShortDateTime helper.

Copy link

@katarzynazawada katarzynazawada left a comment

Choose a reason for hiding this comment

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

🎉

@lserwatka lserwatka merged commit b63d259 into ezsystems:master Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants