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.

Sign up for GitHub

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

Jump to bottom

Select/reset button for events #55

Merged
merged 14 commits into from
Jan 15, 2022
Merged

Select/reset button for events #55

merged 14 commits into from
Jan 15, 2022

Conversation

OliverBalfour
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://yellow-cliff-008e54a00-55.eastasia.azurestaticapps.net

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://yellow-cliff-008e54a00-55.eastasia.azurestaticapps.net

1 similar comment
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://yellow-cliff-008e54a00-55.eastasia.azurestaticapps.net

@OliverBalfour
Copy link
Contributor Author

Only one regression I found: the agenda section has missing data. I've just fixed this

@OliverBalfour OliverBalfour mentioned this pull request Jan 14, 2022
Closed
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://yellow-cliff-008e54a00-55.eastasia.azurestaticapps.net

@OliverBalfour OliverBalfour changed the title [Draft] Select button Select/reset button for events Jan 14, 2022
@OliverBalfour OliverBalfour mentioned this pull request Jan 14, 2022
Closed
@OliverBalfour OliverBalfour mentioned this pull request Jan 14, 2022
Closed
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://yellow-cliff-008e54a00-55.eastasia.azurestaticapps.net

@pl4nty
Copy link
Owner

pl4nty commented Jan 14, 2022

I'm out this morning, but I'll get to this today, thanks Oliver! Would normally avoid global state/rerenders for performance and complexity, but seems fine here (even faster than the FC API anecdotally) and I don't see the state growing too much.

pl4nty
pl4nty requested changes Jan 15, 2022
View reviewed changes
Copy link
Owner

@pl4nty pl4nty left a comment

Choose a reason for hiding this comment

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

Not sure how I feel about the state handling, but it works and the rest is great so I'm happy.

src/utils.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
src/Export.js Show resolved Hide resolved
src/Toolbar.js Outdated Show resolved Hide resolved
src/Toolbar.js Outdated Show resolved Hide resolved
src/Calendar.js Outdated Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://yellow-cliff-008e54a00-55.eastasia.azurestaticapps.net

1 similar comment
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://yellow-cliff-008e54a00-55.eastasia.azurestaticapps.net

@pl4nty
Copy link
Owner

pl4nty commented Jan 15, 2022 •
edited
Loading

Interesting bug with the text wrapping, also affects buttons

image

@OliverBalfour
Copy link
Contributor Author

Re: state handling, the reason I moved everything into App was because a lot of stuff didn't make sense inside Toolbar, eg which events are selected, and I needed a way to propagate that info into the Calendar. The state handling is not ideal, I agree, I only did precisely enough refactoring to enable me to pass the state where it was needed, but didn't clean it up afterwards.

Virtually all of the state handling code is the same or very similar, it's just been moved and is all passed down

@OliverBalfour
Copy link
Contributor Author

Interesting bug with the text wrapping, also affects buttons

image

Good catch, just fixed (missing overflow-y: hidden)

@pl4nty
Copy link
Owner

pl4nty commented Jan 15, 2022

All good, originally I stored all user input in the toolbar (and using the calendar ref just to display), but that fell apart when the calendar itself took input. Glad we're on the same page, not ideal as-is but a big improvement on what I had, and enough to release to users.

@pl4nty
Copy link
Owner

pl4nty commented Jan 15, 2022

Nice! TIL that exists

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://yellow-cliff-008e54a00-55.eastasia.azurestaticapps.net

@OliverBalfour
Copy link
Contributor Author

OliverBalfour commented Jan 15, 2022 •
edited
Loading

Thinking about the state management, the main reason it's ugly is because CalendarApi is imperative and doesn't let you pass an events list prop. Longer term, I might make a FullCalendar wrapper that simply adds an events prop that wrangles CalendarApi for you. Then the only complex bit is generating the list of events to pass via props - that's most of the contents of the giant useEffect - but it could be moved to a helper function in Calendar.js when we can pass events as a prop

@pl4nty
Copy link
Owner

pl4nty commented Jan 15, 2022 •
edited
Loading

ahhhhh, I thought FC was declarative? never tested cause I was attempting to wrangle the API, which was a poor choice in hindsight. https://fullcalendar.io/docs/initialEvents seems to imply the default is declarative, with an imperative escape hatch.

(edit, swapped imperative/declarative by accident lol)

@OliverBalfour
Copy link
Contributor Author

OliverBalfour commented Jan 15, 2022 •
edited
Loading

ahhhhh, I thought FC was imperative? never tested cause I was attempting to wrangle the API, which was a poor choice in hindsight. https://fullcalendar.io/docs/initialEvents

Yeah what I'm proposing is a wrapper that makes a prop like initialEvents, except it actually does update the events when the prop changes. It'd be a separate file that does some funky stuff with effects to translate changes to the prop into CalendarApi calls, so it abstracts away the imperative-ness and presents a declarative API

Edit: see your edit now, gotcha. initialEvents isn't declarative unfortunately, it doesn't update the events when the prop changes

@pl4nty
Copy link
Owner

pl4nty commented Jan 15, 2022 •
edited
Loading

Yeah, makes sense but I think the default events prop (eg https://fullcalendar.io/docs/events-array) is declarative, and initialEvents is imperative for those that want it

pl4nty
pl4nty approved these changes Jan 15, 2022
View reviewed changes
Copy link
Owner

@pl4nty pl4nty left a comment

Choose a reason for hiding this comment

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

LGTM ❤

@OliverBalfour OliverBalfour merged commit 1354771 into master Jan 15, 2022
@OliverBalfour OliverBalfour deleted the select-button branch January 15, 2022 03:56
@pl4nty pl4nty mentioned this pull request Jan 15, 2022
Open
@pl4nty pl4nty linked an issue Jan 15, 2022 that may be closed by this pull request
Unselect Tutorial/Workshop etc. Button #44
Closed
@pl4nty pl4nty mentioned this pull request Jan 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pl4nty pl4nty pl4nty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unselect Tutorial/Workshop etc. Button
2 participants
@OliverBalfour @pl4nty