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

Event date facet (DDFFORM-67) #562

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Event date facet (DDFFORM-67) #562

wants to merge 10 commits into from

Conversation

kasperg
Copy link
Contributor

@kasperg kasperg commented Mar 22, 2024

Link to issue

https://reload.atlassian.net/browse/DDFFORM-67

Description

This PR shows how date filtering should appear on content list pages. The PR consists of two parts:

  1. Refactoring of the existing date calendar component to a date range component which shows both the form element and the Flatpickr calendar.
  2. Extracting the JavaScript from the date range component to something that can be used by systems relying on the design system.

Please go through the individual commits for additional commentary.

Screenshot of the result

billede

Additional comments or questions

I have added a few comments in the PR below.

kasperg added 9 commits March 19, 2024 13:56
Generally we do not use PascalCase for story titles.
This allows us to make the component usable in more situations - e.g.
show the calendar with a form field.

This also allows us to move styling that was currently placed in the
pause modal component into the generic component.

Styling specific to Flatpickr has been moved to a separate SCSS file.
This allows us to reuse the logic in the CMS.
Filters have black borders.

Add a story to showcase this.
This goes to the right of the filters on desktop.

We also want to control the width. Instead of setting the width on the
element like we do for dropdowns we instead set it on the surrounding
elements. This makes it easier to use across different contexts.
Localization will not work inside storybook so this is the best we
can do.

To use this format we have to set an alt input. This requires updates
to our asynchronous clicking as there are now multiple input elements
from Flatpickr.

This also allows us to lower the waiting time before we click.
By splitting the two we can create a date range for content that
has been loaded asynchronously.
The component gets its bottom margin from the origin implementation
but this is not needed here.
@kasperg kasperg marked this pull request as ready for review March 22, 2024 11:55
Comment on lines +32 to +35
now,
animate: false,
altInput: true,
altFormat: "j. M Y",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should align it to the right and not left.
Maybe this is the correct change, not sure.

Suggested change
now,
animate: false,
altInput: true,
altFormat: "j. M Y",
now,
animate: false,
altInput: true,
altFormat: "j. M Y",
position: "auto right",

Comment on lines +80 to +84
<input
className="date-range__input"
type="text"
aria-label="Vælg dato"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Placeholder værdi

Suggested change
<input
className="date-range__input"
type="text"
aria-label="Vælg dato"
/>
<input
className="date-range__input"
type="text"
aria-label="Vælg dato"
placeholder="Vælg dato"
/>

Copy link
Contributor

@LasseStaus LasseStaus left a comment

Choose a reason for hiding this comment

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

Comments reflect a mix of review on the design system, and the cms PR.

General suggestions

  • I found this design for the date picker. I think that is favorable to the empty input so i feel we should add that.

  • I added some comments for placeholder and position/alignment of the open flatpickr, aswell as possibility of showing 2 months in desktop.

  • I think we it would be nice with a reset/clear button for this. A button with an X or some text that shows up left of the date filter.

Potential problematic behaviour

These are not things i think needs to change for now necessarily, but i wanted to highlight some of the flatpickr behaviour that could be problematic.

  • There is no default display of a date. The input is empty, but we could display the current day and forward here since that is what it is showing.

  • If i want to select a date_from, and not a date_to, this is hard. I have to select 1 date, move out of the box without select a range and click. If then i reopen it, the date picker is now expecting a date_to input from the previous interaction.

I am not sure how to prevent it from expecting a date_to on reopening.

  • Related to above: If i again want to select a date_from, and not a date_to, i can also i double click a date. This activates a search with both a date_to and date_from.
  1. I think this is not intuitive.
  2. The filter-input is not diplaying this selection in the format of "Date_from TIL date_to" which i guess is expected.

One suggestion here could be to just handle this by checking if the same date was clicked twice, and then perform a Date_from without date_to search.

Suggestions

Perhabs most of this can be solved with some conditional checking in the current implementation. The date filtering works, but some of the current flatpickr functionality can be confusing.

An alternativ could be to display the date_from and date_to inputs above the calendar, which all would then be in one container that opened. Maybe in a readonly state. The point would be to reflect changes more intuitively to the user, but not sure if necessary.

Take what you wish from this. I think the reset button and individual comments are the most prevalent.

Also i am interested in knowing what made you not do it as a react app 👂

@LasseStaus LasseStaus assigned LasseStaus and kasperg and unassigned LasseStaus Mar 22, 2024
Comment on lines +31 to +32
window.flatpickrOptions = {
now,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to use the functionality built in of displaying 2 months instead of one on desktop / when there is room for it.

Up to you

Suggested change
window.flatpickrOptions = {
now,
window.flatpickrOptions = {
now,
showMonths={2}

Base automatically changed from event-facets to release/form-sprint-9 March 22, 2024 14:45
Base automatically changed from release/form-sprint-9 to develop April 2, 2024 11:40
@kasperg kasperg changed the base branch from develop to release/form-sprint-10 April 2, 2024 11:43
Base automatically changed from release/form-sprint-10 to develop April 8, 2024 13:10
@kasperg kasperg changed the base branch from develop to release/hermod-sprint-12 April 16, 2024 07:53
Base automatically changed from release/hermod-sprint-12 to develop April 23, 2024 06:53
@kasperg kasperg changed the base branch from develop to release/hermod-14 April 30, 2024 14:08
Base automatically changed from release/hermod-14 to develop May 2, 2024 14:12
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.

2 participants