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

Date and time fields use browser-default selection widgets #4202

Merged
merged 11 commits into from
May 30, 2024

Conversation

joemull
Copy link
Member

@joemull joemull commented May 24, 2024

This closes #4095 as documented in that issue. Also closes #4205

image

The pub date selector was the only date+time combination:

Screenshot from 2024-05-24 18-10-28

In adding the form for selecting a pub date, I fixed up the confirmation message a bit so that it is crystal clear even with a timezone difference:

Screenshot from 2024-05-24 18-10-44

To make the datetime picker work, I used a custom model field as documented on the Django forum as discussed.

But as a side effect, an error is thrown when saving the model in the admin. (Change the Article admin back to admin.ModelAdmin to see the error.) To handle this I created an override to use the same widget in the admin. It is a bit ugly:

Screenshot from 2024-05-24 18-39-57

If we don't like this we can simply drop the ModelField and use the custom FormField or widget independently.

@joemull joemull requested review from ajrbyers and mauromsl May 24, 2024 17:19
@joemull joemull removed the request for review from ajrbyers May 24, 2024 17:20
@joemull joemull force-pushed the 4095-standard-datepicker branch from 60c632a to 9fa0397 Compare May 24, 2024 17:35
@joemull joemull changed the base branch from master to release_1_5_x May 24, 2024 17:35
@joemull joemull marked this pull request as ready for review May 24, 2024 17:37
@joemull
Copy link
Member Author

joemull commented May 24, 2024

image

@ajrbyers ajrbyers mentioned this pull request May 28, 2024
@joemull joemull removed the request for review from mauromsl May 29, 2024 08:33
@joemull joemull assigned ajrbyers and unassigned mauromsl May 29, 2024
@joemull joemull requested a review from ajrbyers May 29, 2024 08:33
Copy link
Member

@ajrbyers ajrbyers left a comment

Choose a reason for hiding this comment

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

Generally this is a very welcome change. I've added a couple of questions inline though.

@@ -1016,6 +1017,7 @@ def publish_article(request, article_id):
:param article_id: Article PK
:return: contextualised django template
"""
from submission import forms as submission_forms
Copy link
Member

Choose a reason for hiding this comment

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

Does having this import in the head cause a circular import?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I believe so. There is another function in this file that has the same import.

else:
modal = 'pubdate'
Copy link
Member

Choose a reason for hiding this comment

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

Is this unset on purpose? When the form is invalid generally we'd want to open the modal again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I unset it because there is a general redirect that resets the view and keeps the modal = 'pubdate' from having any effect, and I didn't want to touch the redirect because I'd have to test all the other view branches:

https://github.com/BirkbeckCTP/janeway/blob/e740dbc8ce197360d8cf3e527267b1d7f8d8f133/src/journal/views.py#L1205-L1210

@@ -524,3 +524,19 @@ def utility_clean_orcid(orcid):

# ORCID is None.
return orcid


class PubDateForm(forms.ModelForm):
Copy link
Member

Choose a reason for hiding this comment

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

This is great.

</div>
</div>
</div>
{{ pub_date_form }}
Copy link
Member

Choose a reason for hiding this comment

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

This is being output in table format. Perhaps use as_p or the foundation tag?
Screenshot 2024-05-29 at 13 26 41

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, that is wild. Firefox clears it in my inspector pane (after parsing the HTML) but I can see it when I inspect the source. Is this a Django default feature, or something we have configured? In any case, I've added the foundation filter.

@ajrbyers ajrbyers assigned joemull and unassigned ajrbyers May 29, 2024
@joemull joemull requested a review from ajrbyers May 29, 2024 13:12
@joemull joemull assigned ajrbyers and unassigned joemull May 29, 2024
@joemull joemull linked an issue May 29, 2024 that may be closed by this pull request
@mauromsl mauromsl merged commit 3f326c3 into release_1_5_x May 30, 2024
1 check failed
@mauromsl mauromsl deleted the 4095-standard-datepicker branch May 30, 2024 16:15
@mauromsl mauromsl added this to the v1.5.5 milestone May 30, 2024
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.

Pubdate selector bug JQUI datepicker interferes with TinyMCE toolbar
3 participants