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

allow dates to be specified as a year only #921

Closed
wants to merge 1 commit into from
Closed

allow dates to be specified as a year only #921

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 10, 2021

this commit allows a user to specify a books publication date as a year only
year-only dates are represented as january first of that year
a boolean flag identifies a publication date as year-only or as a full date
currently, checking the "year only" box (frontend) but submitting a regular date value will fail to save the book
closes #743

making this a draft because i'm not happy with a few things, but not sure the best way to fix them. i'll be working on them some more, but i would definitely welcome input on everything here :)

bookwyrm/forms.py: not sure if directly mutating the querydict is a great idea here. someone in stackoverflow mentioned it, but django only mentions making a copy and mutating that. also, this init function will cause a form submission to fail if a user doesn't have js on, clicks the yearbox, and then enters a date in the datepicker. i'll fix that shortly though

bookwyrm/templates/book/edit_book.html lines 7-45: the event listeners are kinda gross. i'm not sure a better way that doesn't involve a lot of boilerplate/magic strings/duplicated code though, suggestions definitely welcome

bookwyrm/templates/book/publisher_info.html: i wasn't really sure how to work with the blocktrans bit. the only thing i can think is to extract that into its own tag, or potentially a separate template macro file

bookwyrm/views/books.py line 185: i'm not entirely sure of the logic behind a request flow for this view, so i'm not sure if ignoring those exceptions is a good idea or not

this commit allows a user to specify a books publication date as a year only
year-only dates are represented as january first of that year
a boolean flag identifies a publication date as year-only or as a full date
currently, checking the "year only" box (frontend) but submitting a regular date value will fail to save the book
@arkhi
Copy link
Contributor

arkhi commented Apr 13, 2021

I would also like to have the possibility to have incomplete dates. As my understanding goes, the ISO8601 provides a lot of leeway to what format is acceptable; 2021, 2021-04, 2021-04-13 or even --04-13 all should work.

I wonder if changing the python and db logic to allow any valid format wouldn’t be a better and more sturdy solution than provide more options to the user and the exceptions that entails.

Considering the input is only a text input when the date type is not recognized, I understand the input is stored as a string. I didn’t check what the conditions are for the string to be sent in the request though, maybe the work could be focused on that?

@@ -4,6 +4,45 @@

{% block title %}{% if book %}{% blocktrans with book_title=book.title %}Edit "{{ book_title }}"{% endblocktrans %}{% else %}{% trans "Add Book" %}{% endif %}{% endblock %}

{% block scripts %}
<script defer>
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on where this PR is gonna go, I can help you with JS if you’re seeking assistance; just ping me if you wish.

Just two things for the sake of sharing:

Copy link
Author

Choose a reason for hiding this comment

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

i appreciate that! i'll definitely ping if something comes up. i used an inline script just for development, but i didn't know that about defer, thank you!

Comment on lines +30 to +39
// potential HACK: older ie browsers require a different method call: https://gist.github.com/tzi/2953155#file-important-js-L5
if (event.target.checked) {
first_published_date_label.setAttribute("style", "display: none !important");
first_published_year_label.setAttribute("style", "display: block");
first_published_date_input.type = "number";
} else {
first_published_date_label.setAttribute("style", "display: block");
first_published_year_label.setAttribute("style", "display: none !important");
first_published_date_input.type = "date";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Your comment raises an interesting question: Does BookWyrm want to define a set of supported browser for a full-feature experience?

If yes, which ones? (That would definitely go into a .browserslistrc file. :) )

Copy link
Author

Choose a reason for hiding this comment

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

my personal projects are always "the browsers supported are whatever browsers support the features i need without polyfills and work-arounds". that's definitely not accessible enough for a project with more reach and dev power though (imo)

i recently read an article (that i've lost, sadly) talking about how few people actually use internet explorer. my personal opinion is that ie is end-of-life for several years now, bookwyrm's potential userbase probably has no overlap with any of the people using ie, and the added size of polyfills and work-arounds is more of an accessibility issue than one or two people being unable to use certain js features

as for non-ie browsers to be supported, i'd have to defer to you! i think the js here makes it pretty clear that i'm not a front-end person :p

Copy link
Contributor

Choose a reason for hiding this comment

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

I completely second the progressive enhancement approach.

My question was more about a full-feature experience, where feature meant UX/UI features more than the content provided. I hope the intention is clearer. :)

I won’t regret not caring about IE specifically as long as the content is available to the few people using it.

Copy link
Author

Choose a reason for hiding this comment

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

i definitely understand what you mean now! i think it might be a good idea to explicitly list several target browsers, so we can make sure we're usable with more than just the main two or three

@ghost
Copy link
Author

ghost commented Apr 13, 2021

@arkhi

I would also like to have the possibility to have incomplete dates.

i think having the ability to do year and month (but no day) could be useful, but i'm not sure how useful any other date representation might be. if we do want to be able to set the published date to year and month or year only, i'd need suggestions for how to structure that ui since i have no experience with ui design

I wonder if changing the python and db logic to allow any valid format wouldn’t be a better and more sturdy solution than provide more options to the user and the exceptions that entails.

i've actually been holding off on working on this because i've been kicking around creating a custom widget for thisk it seems like the idiomatic django thing to do. i originally held off on it because i thought it would be too much work, but after #893 i think that's the best route to go. been meaning to ask what others think but haven't had a chance ^^"

Considering the input is only a text input when the date type is not recognized, I understand the input is stored as a string. I didn’t check what the conditions are for the string to be sent in the request though, maybe the work could be focused on that?

django's DateField uses a datetime type for a python representation, but by default uses a text input widget, and stores the date as a "timestamp with timezone" in the db

@arkhi
Copy link
Contributor

arkhi commented Apr 14, 2021

i think having the ability to do year and month (but no day) could be useful, but i'm not sure how useful any other date representation might be. if we do want to be able to set the published date to year and month or year only, i'd need suggestions for how to structure that ui since i have no experience with ui design

(I wish the input[type=date] HTML element would not be so restrictive that one has to input the whole date.)

I’m brainstorming here… Please don’t take my comment as a Do this; the ones that implement win. :)

I can see the following options:

  • use a input[type=date] and a switch to show or hide different elements (your proposal);
  • use a input[type=text] with a script to validate the date and that provides a unified UI;
  • use a input[type=date] and a switch to toggle the attributes of the existing input.

The value I see with the third option is that we don’t add more checks for the same book property. The logic of defining the format happens in the frontend with one input (and a checkbox) and the validation logic in the backend is only checking the one value submitted. This option could do something like the following, with I as the user:

  1. If the browser supports the date type, I can check a partial date checkbox.

  2. Some <input> attributes are updated:

    • type is changed to text.
    • pattern="^[0-9]{4}(-(0[1-9]|1[0-2])(-(0[1-9]|[1-2][0-9]|3[0-1]))?)?$" is added. (better patterns are welcome)

    A help also shows me the expected format with the current date as example: example: 2021-04-14.

  3. I can type whatever I want that matches the pattern, then submit.

  4. The backend validator makes sure I didn’t input an invalid date (like 2021-02-29) and lets me know if I did.

@arkhi
Copy link
Contributor

arkhi commented Apr 14, 2021

django's DateField uses a datetime type for a python representation, but by default uses a text input widget, and stores the date as a "timestamp with timezone" in the db

I will let people who know Django jump in. :)

@arkhi
Copy link
Contributor

arkhi commented Apr 14, 2021

pattern="…"

I updated the pattern to ^[0-9]{4}(-(0[1-9]|1[0-2])(-(0[1-9]|[1-2][0-9]|3[0-1]))?)?$.

@ghost
Copy link
Author

ghost commented Apr 15, 2021

i like your partial date and pattern idea. it seems simple and straight to the point. i think that's what django's datefield widget might default to, so it shouldn't be too difficult (famous last words) to subclass it and add in the partial date checkbox bit

@ghost ghost closed this Apr 17, 2021
This pull request was closed.
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.

Allow dates to be only year
1 participant