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

Format daterange filter input values according to i18n default format #8209

Closed
wants to merge 8 commits into from

Conversation

eikes
Copy link
Contributor

@eikes eikes commented Jan 5, 2024

When my locale is de, I expect my date format to be %d.%m.%Y. The current behavior forces a different format. This PR makes sure, that the default format is used. Just like the formtastic date input does.

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (860b024) 99.10% compared to head (861b816) 99.10%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8209   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files         140      140           
  Lines        4017     4018    +1     
=======================================
+ Hits         3981     3982    +1     
  Misses         36       36           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eikes
Copy link
Contributor Author

eikes commented Jan 5, 2024

Or might I even suggest to split the method and change it like this?

        def input_html_options_for(input_name, placeholder)
          { placeholder: placeholder,
            value: input_value_for(input_name) }.merge(input_html_options)
        end

        def input_value_for(input_name)
          @object.public_send(input_name).to_date.to_s
        rescue
          ""
        end

Co-authored-by: Geremia Taglialatela <tagliala@users.noreply.github.com>
Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

@eikes thank you. It would help to have a feature test for this. We have a locale step ("set my locale to") so you should be able to test this with a Capybara assertion that the input value is now set to expected format for the locale.

@eikes
Copy link
Contributor Author

eikes commented Jan 7, 2024

@javierjulio I created a test and the gherkin-linter is happy now as well.

Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

@eikes does this work as intended for you locally with the generated app? Granted, I haven't loaded a different locale before but tried with this and I'm not seeing the change, unless I'm misunderstanding it. Although, seems that changing the language on the page will not suffice to see it in the browser which would explain why the date input appears the same for me. Just want to be sure it's working as intended for you. If so, then we'll get this in. Thanks!

@@ -35,6 +35,20 @@ Feature: Index Filtering
And I press "Filter"
Then I should see current filter "title_cont" equal to "<script>alert('hax')</script>" with label "Title contains"

Scenario: Daterange Filters Uses Locale Format
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
Scenario: Daterange Filters Uses Locale Format
Scenario: Date range filters use locale format

@javierjulio
Copy link
Member

@eikes can you describe a bit more why you want to make this change and for which version? Note that the default branch of ActiveAdmin is for v4 and we've removed jQuery UI and its date picker. Any date inputs will now be the native HTML date input. From what I can tell, the date input parsed value is always in the same format of yyyy-mm-dd but the display will be based on the browser's locale so if anything we wouldn't want to make this change and leave the previous format as it was. Are we missing something?

@javierjulio
Copy link
Member

@eikes from what we can tell, what we do currently is meant to be the way so I'm going to close this out. Thanks. Feel free to continue any discussion.

@eikes
Copy link
Contributor Author

eikes commented Jan 13, 2024

I wasn‘t aware of the switch to the native datepicker. The PR is not needed in this case.

@javierjulio
Copy link
Member

@eikes that makes sense. No worries. We figured that was the case. Thank you for the follow up and confirmation.

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.

None yet

3 participants