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-4791: [Date picker] Cannot open date time picker by clicking on calendar icon in the input #678

Merged
merged 6 commits into from
Feb 15, 2023
Merged

IBX-4791: [Date picker] Cannot open date time picker by clicking on calendar icon in the input #678

merged 6 commits into from
Feb 15, 2023

Conversation

GrabowskiM
Copy link
Contributor

@GrabowskiM GrabowskiM commented Jan 16, 2023 •
edited
Loading

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

With: https://github.com/ibexa/page-builder/pull/193

Checklist:

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

@GrabowskiM GrabowskiM self-assigned this Jan 16, 2023
tischsoic
tischsoic reviewed Jan 17, 2023
View reviewed changes
src/bundle/Resources/public/scss/_date-time-picker.scss Outdated
cursor: pointer;

.ibexa-icon {
fill: $ibexa-color-primary;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add the animation property? Maybe this should be a modified ghost button like in the case of other action buttons from src/bundle/Resources/views/themes/admin/ui/component/input_text.html.twig? 🤔

tischsoic
tischsoic reviewed Jan 17, 2023
View reviewed changes
src/bundle/Resources/views/themes/admin/ui/component/inputs/input_date_time_picker.html.twig Outdated
@@ -27,7 +27,7 @@
{% endblock %}
{% block actions %}
{{ parent() }}
<div class="ibexa-input-text-wrapper__action-btn">
<div class="ibexa-input-text-wrapper__action-btn ibexa-input-text-wrapper__action-btn--calendar-icon">
Copy link
Contributor

@tischsoic tischsoic Jan 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just thinking: I think that the initial idea was that it is not a button, but rather a regular icon. In such a case, the problem is not that it's not the button, but that this icon is blocking pointer events on the input. 🤔
Should this behave like a button? Wouldn't it be confusing given you can now open the picker using both input and this button? 🤔

tischsoic
tischsoic reviewed Jan 18, 2023
View reviewed changes
src/bundle/Resources/public/js/scripts/core/date.time.picker.js Outdated
class DateTimePicker {
constructor(config) {
this.container = config.container;
this.fieldWrapper = this.container.querySelector('.ibexa-date-time-picker');
this.inputField = this.fieldWrapper.querySelector('.ibexa-date-time-picker__input');
this.actionsWrapper = this.fieldWrapper.querySelector('.ibexa-input-text-wrapper__actions');
this.calendarButton = this.actionsWrapper.querySelector('.ibexa-input-text-wrapper__action-btn--calendar');
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick

Suggested change
this.calendarButton = this.actionsWrapper.querySelector('.ibexa-input-text-wrapper__action-btn--calendar');
this.calendarBtn = this.actionsWrapper.querySelector('.ibexa-input-text-wrapper__action-btn--calendar');

src/bundle/Resources/views/themes/admin/ui/component/inputs/input_date_time_picker.html.twig Outdated
@@ -27,10 +27,10 @@
{% endblock %}
{% block actions %}
{{ parent() }}
<div class="ibexa-input-text-wrapper__action-btn">
<button type="button" class="btn ibexa-btn ibexa-btn--ghost ibexa-btn--no-text ibexa-input-text-wrapper__action-btn ibexa-input-text-wrapper__action-btn--calendar">
Copy link
Contributor

Choose a reason for hiding this comment

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

can we split it into multiple lines? :)

@bogusez bogusez self-assigned this Jan 23, 2023
@bogusez
Copy link
Contributor

bogusez commented Jan 24, 2023

@GrabowskiM I cannot open date time picker for airtime modal (collection block - PB)
Screenshot 2023-01-24 at 08 19 17

There are also some JS exceptions when I select a content: https://recordit.co/ltArbgqxvE

This was referenced Feb 2, 2023
Closed
Closed
@bogusez
Copy link
Contributor

bogusez commented Feb 2, 2023

Regression tests passed:
ibexa/experience#134
ibexa/commerce#208

@sonarcloud
Copy link

sonarcloud bot commented Feb 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bogusez
Copy link
Contributor

bogusez commented Feb 15, 2023

Regression tests passed on recent changes:
ibexa/experience#134
ibexa/commerce#208

@dew326 dew326 merged commit 6d2074a into 4.3 Feb 15, 2023
@dew326 dew326 deleted the IBX-4791-calendar-clickable branch February 15, 2023 12:07
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

@Gengar-i Gengar-i Gengar-i approved these changes

@bogusez bogusez bogusez approved these changes

Assignees

@mikadamczyk mikadamczyk

@GrabowskiM GrabowskiM

@Nattfarinn Nattfarinn

@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.
8 participants
@GrabowskiM @bogusez @tischsoic @dew326 @lucasOsti @Gengar-i @mikadamczyk @Nattfarinn