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

fix: invalid date_time #2279

Merged
merged 14 commits into from Jan 4, 2024
Merged

fix: invalid date_time #2279

merged 14 commits into from Jan 4, 2024

Conversation

Paul-Bob
Copy link
Contributor

@Paul-Bob Paul-Bob commented Dec 21, 2023

Description

Fields of type has_one was eager loading on tabs.

This was causing the date_time field's related JS to broke when the field panel was loading on a hidden tab, resulting on the string Invalid DateTime.

More specifically the this.initialValue on the date field stimulus controller was getting some weird spaces around the value when the field was rendering hidden, we also trim that value now.

Fixes #2261

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots

Manual review steps

  1. Step 1
  2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

@Paul-Bob Paul-Bob added the Fix label Dec 21, 2023
@Paul-Bob Paul-Bob self-assigned this Dec 21, 2023
Copy link

codeclimate bot commented Dec 21, 2023

Code Climate has analyzed commit 6b59894 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Collaborator

@adrianthedev adrianthedev left a comment

Choose a reason for hiding this comment

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

Could you please add some tests and a scenario where one can try this?

@Paul-Bob
Copy link
Contributor Author

Paul-Bob commented Jan 3, 2024

Could you please add some tests and a scenario where one can try this?

Done, the bug was happening when a has_one field was rendering on a secondary tab. Added the comment on user resource that contains a date time field to simulate the bug.

@adrianthedev
Copy link
Collaborator

Could you please add a regular date_time in a tab and see if it works? I think it breaks without the has_one.

@Paul-Bob
Copy link
Contributor Author

Paul-Bob commented Jan 3, 2024

Could you please add a regular date_time in a tab and see if it works? I think it breaks without the has_one.

You're right, the issue is the same, that tab is eager loading, since is not an association I'm not sure if it's viable to lazy load it. I'm checking the JS maybe we can avoid breaking from there

@Paul-Bob
Copy link
Contributor Author

Paul-Bob commented Jan 3, 2024

The initial value on JS has some white spaces around it when the field was rendering hidden. We trim that value now

module FrameLoading
extend ActiveSupport::Concern

def turbo_frame_loading
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think this change will impact any other components @Paul-Bob?

Copy link
Contributor Author

@Paul-Bob Paul-Bob Jan 4, 2024

Choose a reason for hiding this comment

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

we should also apply this change on app/components/avo/fields/has_many_field/show_component.html.erb

@Paul-Bob Paul-Bob merged commit 94f6e77 into main Jan 4, 2024
17 checks passed
@Paul-Bob Paul-Bob deleted the fix/invalid_date_time branch January 4, 2024 12:00
Copy link
Contributor

github-actions bot commented Jan 4, 2024

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.

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

Successfully merging this pull request may close these issues.

Invalid DateTime
2 participants