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

Draft: Work in progress adding filter to customize Default DurationOptions. #882

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

Conversation

ajskelton
Copy link
Contributor

@ajskelton ajskelton commented Sep 17, 2024

The filter works, but something in the DateTimeRange useEffect is reverting back to the default values.

Description of the Change

Closes #876

How to test the Change

Changelog Entry

Added - New feature

Credits

Props @ajskelton

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

The filter works, but something in the DateTimeRange useEffect is reverting back to the default values.
Copy link
Contributor

@mauteri mauteri left a comment

Choose a reason for hiding this comment

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

Thanks for contributing, Anthony! Take a look at my comments and let me know what you think.

},
];

export default applyFilters('gatherpress.durationOptions', DurationOptionsDefaults );
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put this back in helpers/datetime.js, but adjust it a bit. This area is for React components, this is just returning an object.

* @property {string} label - The human-readable label for the duration option.
* @property {number|boolean} value - The value representing the duration in hours, or `false` if a custom end time is to be set.
*/
export const durationOptions = [
Copy link
Contributor

Choose a reason for hiding this comment

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

let's keep the name durationOptions, but convert this to a function. Then apply your filter to the function before returning.

@@ -126,7 +90,7 @@ export function dateTimeOffset(hours) {
*/
export function getDateTimeOffset() {
return (
durationOptions.find(
DurationOptions.find(
Copy link
Contributor

Choose a reason for hiding this comment

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

After changes above this would become durationOptions().find(

@@ -35,11 +36,11 @@ const Duration = () => {
<SelectControl
label={__('Duration', 'gatherpress')}
value={
durationOptions.some((option) => option.value === duration)
DurationOptions.some((option) => option.value === duration)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will become durationOpions().some... after changes below.

? duration
: false
}
options={durationOptions}
options={DurationOptions}
Copy link
Contributor

Choose a reason for hiding this comment

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

options={durationOptions()}

* Moves the object back into the datetime helpers file
* Converted to a function that returns the filtered values
* Updated usage of durationOptions
@ajskelton
Copy link
Contributor Author

The comments made a lot of sense and I pushed up a new commit with the updates. With those in place the filter from a theme was working great, but there is one bug that I was able to drill down.

When the page loads the gatherpress/datetime store builds the DEFAULT_STATE. The duration value uses the getDateTimeOffset() function that calculates what the duration is. During this first instance the durationOptions() function does not use the filtered values from the theme, it still uses the defaults. When it doesn't find the corresponding value in the object returned from durationOptions it returns false. With the state set to false it doesn't render the dropdown and instead shows the Date and Time End field so you can't change the duration anymore.

Do you think it would be better to save the duration value to the post meta along with the start and end dates so this calculation doesn't need to happen?

@mauteri
Copy link
Contributor

mauteri commented Sep 26, 2024

The comments made a lot of sense and I pushed up a new commit with the updates. With those in place the filter from a theme was working great, but there is one bug that I was able to drill down.

When the page loads the gatherpress/datetime store builds the DEFAULT_STATE. The duration value uses the getDateTimeOffset() function that calculates what the duration is. During this first instance the durationOptions() function does not use the filtered values from the theme, it still uses the defaults. When it doesn't find the corresponding value in the object returned from durationOptions it returns false. With the state set to false it doesn't render the dropdown and instead shows the Date and Time End field so you can't change the duration anymore.

Do you think it would be better to save the duration value to the post meta along with the start and end dates so this calculation doesn't need to happen?

Hey @ajskelton, can you try to call this function to get the duration value? This should serve the same purpose you are looking for with storing meta. Basically, it takes the start/end datetime and returns the duration or false (same as you'd get from the meta) https://github.com/GatherPress/gatherpress/blob/main/src/helpers/datetime.js#L127 Let me know if that works. Thx!

@ajskelton
Copy link
Contributor Author

@mauteri This is the function that doesn't use the filtered values when the default state is being set.

I tested by adding some console.logs in the .find(). When you refresh the page and select the block you see the default duration values logging to the console instead of the filtered duration.

So it sets the duration to false before checking against the filtered options.

When it runs here:

duration: getDateTimeOffset(),
, it must be running before the filter in the theme is registered.

@mauteri
Copy link
Contributor

mauteri commented Sep 26, 2024

@mauteri This is the function that doesn't use the filtered values when the default state is being set.

I tested by adding some console.logs in the .find(). When you refresh the page and select the block you see the default duration values logging to the console instead of the filtered duration.

So it sets the duration to false before checking against the filtered options.

When it runs here:

duration: getDateTimeOffset(),

, it must be running before the filter in the theme is registered.

Oh gotcha, let me give it some thought.

@carstingaxion
Copy link
Collaborator

Hello @ajskelton & @mauteri,

are you aware of WordPress/gutenberg#60971, which seems slightly related?

Sets the initial duration value to null instead of using the getDateTimeOffset function.
This function is moved to the getDuration selector. This selector is run in the Duration component which renders after any theme filters have been registered. We do need to check if the value is false when the user selects the value to custom pick the end time.
@ajskelton
Copy link
Contributor Author

@mauteri I found a fix for the issue. Instead of running the getDateTimeOffset() function in the DEFAULT_STATE, i changed that initial duration value to be null. Then I moved that function to the getDuration selector. When the Duration Component renders it always runs the getDuration selector to dynamically get the value based on the start and end times. And when this runs all of the theme filters have already been registered.

We do need to check if the current state is false here in the selector. I first tried just using the getDateTimeOffset but when the value was set to false the state wasn't saving the false value and the DateTimeEnd was never rendering.

I don't have a ton of experience working with custom stores so let me know if this is the wrong way to go about this.

@carstingaxion That new Date/Time Range component does look nice. I think it's still just design right now, I couldn't see any code commits to check out.

@@ -78,7 +78,7 @@ const store = createReduxStore('gatherpress/datetime', {
selectors: {
getDateTimeStart: (state) => state.dateTimeStart,
getDateTimeEnd: (state) => state.dateTimeEnd,
getDuration: (state) => state.duration,
getDuration: (state) => state.duration === false ? false : getDateTimeOffset(),
Copy link
Contributor

Choose a reason for hiding this comment

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

getDuration: (state) => false === state.duration ? false : getDateTimeOffset(),

Copy link

Preview changes with Playground

You can preview the recent changes for PR#882 with the following PHP versions:

PHP Version 8.3

PHP Version 7.4

Download .zip with build changes

Made with 💙 from GatherPress & a little bit of WordPress Playground. Changes will not persist between sessions.

@mauteri mauteri changed the base branch from main to develop October 10, 2024 12:59
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.

Make default durations filterable
3 participants