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

#4051: Flexible DateTime widget: Small UX improvement when clearing date #4053

Closed

Conversation

stefan-korn
Copy link
Contributor

@stefan-korn stefan-korn commented Nov 7, 2023

fixes #4051

  • Test coverage exists
  • Documentation exists

QA Steps

  • Clearing a date with time part "00:00:00" should allow to save and clear the time part while processing the value. Can use Release Date in dataset for example.

Copy link
Member

@dafeder dafeder left a comment

Choose a reason for hiding this comment

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

Looks good @stefan-korn technically missing tests but i don't think that widget has test coverage to begin with so I won't consider it a blocker.

The thing codeclimate is complaining about is this unnecessary nested conditional:

    if (!empty($input['time']) && $element['#date_date_element'] !== 'none' && empty($input['date'])) {
      if ($input['time'] === '00:00:00') {

At least we should combine all three conditions into a single if, but I think two of them are redundant anyway, so I think this should produce the same result:

if (
  ($input['time'] ?? NULL === '00:00:00') 
  && $element['#date_date_element'] !== 'none' 
  && empty($input['date'])
) {

@stefan-korn
Copy link
Contributor Author

@dafeder : Okay, I changed the condition like you proposed. I am totally having a hard time understanding this right away

$input['time'] ?? NULL === '00:00:00'

and assume that others might have too. So sometimes I would prefer easy code readability over "code climate", but this is surely opinionated.

@dafeder dafeder self-assigned this Apr 5, 2024
@dafeder
Copy link
Member

dafeder commented Apr 9, 2024

Replacing with #4157

@dafeder dafeder closed this Apr 9, 2024
@dafeder
Copy link
Member

dafeder commented Apr 9, 2024

@dafeder : Okay, I changed the condition like you proposed. I am totally having a hard time understanding this right away

$input['time'] ?? NULL === '00:00:00'

and assume that others might have too. So sometimes I would prefer easy code readability over "code climate", but this is surely opinionated.

Sorry @stefan-korn going to override you on this one :). The null coalesce operator is increasingly encouraged in PHP and can be found throughout DKAN. I recognize it's subjective, but IMO once you've seen it a few times it becomes intuitive and improves the code flow a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flexible DateTime widget: Small UX improvement when clearing date
2 participants