Add Editor Preview and move data fetching to its own context #97
Conversation
senadir
requested review from
StevenDufresne and
ryelle
and removed request for
StevenDufresne
| export function EventDataProvider( { children, data } ) { | ||
| const [ eventData, setEventData ] = useState( [] ); | ||
| // we bail out early if data is hydrated from the server. | ||
| if ( data ) { |
There was a problem hiding this comment.
I think we should keep this in a ref to save the reference between retenders, but I don’t think we will need it unless we move this logic to EventProvider
| const currentDate = new Date(); | ||
| const fromDate = new Date( currentDate.getFullYear(), currentDate.getMonth(), 1 ); |
There was a problem hiding this comment.
This can probably benefit from something inside @wordpress/date
| const fromDate = new Date( currentDate.getFullYear(), currentDate.getMonth(), 1 ); | ||
|
|
||
| apiFetch( { | ||
| path: `/wp/v2/meetings/from/${ format( 'Y-m-d', fromDate ) }?per_page=12`, |
There was a problem hiding this comment.
the value 12 is what I brought from the logic within the PHP code.
There was a problem hiding this comment.
also depending on the size of data 12 per page could mean several months in advance or simply this week, so the API might be changed to some sort of from-to, and we can fetch more months as long the user is changing the page.
| icon="calendar" | ||
| label={ __( 'Go to "Meetings" to add or edit your meetings', 'wporg' ) } | ||
| /> | ||
| <Disabled> |
There was a problem hiding this comment.
disabling interactive blocks within the editor is kinda a force of habit now 😅since some interaction depends on different contexts, I don’t think this is the case in this block so I might remove the disabled wrap, however, it might be confusing to the user if they think the changes they make in the editor will persist on the frontend (changing default month or changing display style).
| // we bail out early if data is hydrated from the server. | ||
| if ( data ) { | ||
| return ( | ||
| <EventDataContext.Provider value={ data }> | ||
| { children } | ||
| </EventDataContext.Provider> | ||
| ); | ||
| } |
There was a problem hiding this comment.
You're breaking a rule of hooks here by bailing early. Instead you should put your condition inside the effect and bail early from the effect instead.
There was a problem hiding this comment.
Wouldn’t be right if I was running the hook inside an else clause? I do agree that having everything inside the useEffect would result in the same result but I guess I'm missing the hook rule part.
There was a problem hiding this comment.
Agree with @nerrad, hooks rules and we can dry up the code by not having to repeat the EventDataContext twice. As he mentioned, just check for data on the first line of the useEffect and return if its hydrated.
|
I understand why your instinct led you to add a new provider but I agree with @nerrad -- we probably don't need it. You can add it to the current |
|
Lastly, Thanks for jumping in on this block! Its always great having more contributors. :) |
Hi!
First, thank you for this wonderful project, I always wanted to build one for a side project and to get to use one is wonderful.
Following my talk with Kelly in slack, she suggested I can contribute.
So this PR does some things, notably:
1- It introduces an Editor preview by simply rendering the App component in the editor.
for this to happen, I needed to create a new Context that fetches the data and pass it to EventProvider, I put it in a separate context to avoid calling the API each time the context updates, but I believe this can be mitigated if we save the fetch value in a ref and only fetch if that ref is
null.2- To keep compatibility, the context also accepts an optional data prop that has the event data, this allows us to hydrate the data from the server to avoid the initial request, using the same logic that was there.
3- This optional data will also allow us to pass a different kind of data to the editor, currently, I'm using real data, this can be swapped by a preview Data that can be passed the same way we pass the hydrated data
4- we move
wporg-calendar-styleregistration to outside admin as well, so that it can render on the editor.I'm also tagging @nerrad for a confidence check since I discussed some of those points with him.