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

IBX-1339: DateTime Widget #297

Merged
merged 13 commits into from
Mar 23, 2022
Merged

IBX-1339: DateTime Widget #297

merged 13 commits into from
Mar 23, 2022

Conversation

GrabowskiM
Copy link
Contributor

@GrabowskiM GrabowskiM commented Feb 2, 2022 •
edited
Loading

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-1339
Bug fix? yes/no
New feature? yes/no
BC breaks? yes/no
Tests pass? yes/no
Doc needed? yes/no
License GPL-2.0

https://github.com/ibexa/scheduler/pull/47
https://github.com/ibexa/page-builder/pull/99
https://github.com/ibexa/personalization/pull/125

Checklist:

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

@GrabowskiM GrabowskiM marked this pull request as draft February 2, 2022 06:52
@GrabowskiM GrabowskiM changed the title temp date time IBX-1339: DateTime Widget Mar 3, 2022
@GrabowskiM GrabowskiM marked this pull request as ready for review March 3, 2022 13:52
@GrabowskiM GrabowskiM force-pushed the IBX-1339-date-time-widget branch 2 times, most recently from ad96dcd to 46e3466 Compare March 8, 2022 11:39
tischsoic
tischsoic requested changes Mar 10, 2022
View reviewed changes
src/bundle/Resources/views/themes/admin/ui/component/input_date_and_time.html.twig Outdated
{% set is_small = is_small|default(false) %}
{% set inline_flatpickr = inline_flatpickr|default(false) %}

{% set wrapper_attr = wrapper_attr|default({})|merge({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these attributes should be called just attr because you are applying them directly on ibexa-date-time-picker.

Suggested change
{% set wrapper_attr = wrapper_attr|default({})|merge({
{% set attr = wrapper_attr|default({})|merge({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly, I'm using here our input_text component, which uses wrapper_attr https://github.com/ibexa/admin-ui/blob/main/src/bundle/Resources/views/themes/admin/ui/component/input_text.html.twig#L4 so without changing it there (or adding extra div here, but it's pointless IMO) it can't be changed to attr

src/bundle/Resources/public/js/scripts/core/date.and.time.js Outdated
init() {
this.flatpickrInstance = flatpickr(this.inputField, this.flatpickrConfig);

this.inputField.addEventListener('input', this.onInputBtn, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a button

Suggested change
this.inputField.addEventListener('input', this.onInputBtn, false);
this.inputField.addEventListener('input', this.onInput, false);

src/bundle/Resources/public/js/scripts/core/date.and.time.js Outdated

onChange(dates) {
const isDateSelected = !!dates[0];
const restArgument = { inputField: this.inputField, flatpickrDates: dates };
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about keeping the name dates in case we would like to e.g. change internals and abandon flatpickr in the future?

Suggested change
const restArgument = { inputField: this.inputField, flatpickrDates: dates };
const otherArguments = { inputField: this.inputField, dates };

src/bundle/Resources/views/themes/admin/ui/component/input_date_and_time.html.twig Outdated
@@ -0,0 +1,36 @@
{% import '@ibexadesign/ui/component/macros.html.twig' as html %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be better to put this file inside its own directory:
component/inputs/input_date_and_time.html.twig
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then we should probably move input_text.html.twig widget as well, WDYT guys? @lucasOsti @dew326

src/bundle/Resources/views/themes/admin/ui/component/input_date_and_time.html.twig Outdated

{% embed '@ibexadesign/ui/component/input_text.html.twig' with { has_search: false } %}
{% block content %}
<input {{ input_attr_rendered }} />
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I think this is simpler and clearer.

Suggested change
<input {{ input_attr_rendered }} />
<input {{ html.attributes(input_attr) }} />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it's not working, it claims that html is not defined

Copy link
Contributor

Choose a reason for hiding this comment

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

You have to import html inside embed.

src/bundle/Resources/views/themes/admin/ui/component/input_date_and_time.html.twig Outdated
{% import '@ibexadesign/ui/component/macros.html.twig' as html %}

{% set is_small = is_small|default(false) %}
{% set inline_flatpickr = inline_flatpickr|default(false) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to avoid using flatpickr in the "interface" of our components. Maybe:

Suggested change
{% set inline_flatpickr = inline_flatpickr|default(false) %}
{% set is_datetime_popup_inline = is_datetime_popup_inline|default(false) %}

src/bundle/Resources/public/js/scripts/core/date.and.time.js Outdated
time_24hr: true,
formatDate: (date) => formatShortDateTime(date, null),
};
class DateAndTime {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe to be consistent with css ibexa-date-time-picker

Suggested change
class DateAndTime {
class DateTimePicker {

@GrabowskiM GrabowskiM force-pushed the IBX-1339-date-time-widget branch 2 times, most recently from 694d31f to fd45594 Compare March 14, 2022 12:26
@bogusez bogusez self-assigned this Mar 17, 2022
@bogusez
Copy link
Contributor

bogusez commented Mar 17, 2022

@GrabowskiM

Screenshot 2022-03-17 at 14 01 25

Screenshot 2022-03-17 at 14 01 02

Screenshot 2022-03-17 at 14 00 44

When input field is empty then 'X' button should be hidden. Take a look on mockup to compare -> https://projects.invisionapp.com/d/main#/console/21342437/456307022/preview

Screenshot 2022-03-17 at 13 56 37

@bogusez
Copy link
Contributor

bogusez commented Mar 22, 2022

@GrabowskiM

Creating new content page. 'X' button have to be removed when input field is empty.

Screenshot 2022-03-22 at 11 17 49

@bogusez
Copy link
Contributor

bogusez commented Mar 22, 2022

Can we adjust the width of date and time input to calendar width?

Screenshot 2022-03-22 at 11 21 13

@dew326 dew326 merged commit 0f17d81 into main Mar 23, 2022
@dew326 dew326 deleted the IBX-1339-date-time-widget branch March 23, 2022 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tischsoic tischsoic tischsoic approved these changes

@dew326 dew326 dew326 approved these changes

@lucasOsti lucasOsti lucasOsti approved these changes

@bogusez bogusez bogusez approved these changes

Assignees

@dew326 dew326

@bogusez bogusez

Labels
QA approved Ready for MERGE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GrabowskiM @bogusez @tischsoic @dew326 @lucasOsti