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

feat(time-grid): allow consumer to set allDayMaxRows (#2386) #2393

Merged
merged 2 commits into from
Jun 2, 2023

Conversation

fornesarturo
Copy link
Contributor

  • Addresses Support "show more" in "All day" in week view #2386
  • Currently react-big-calendar is not limiting the amount of events in the All day row displayed in Week and Day view, if this row grows it may also cover the events and prevent the user from seeing their normal events.

In this commit:

  • Add support for displaying Show more text in TimeGrid views via allDayMaxRows prop.
  • Prevent Background cells from capturing click on show more button.

@cutterbl
Copy link
Collaborator

@fornesarturo First pass looks like it might be good, but we would need a story in storybook for the new prop, and showcasing how it works.

@fornesarturo fornesarturo force-pushed the add-show-more-in-week-view branch from ad8100c to df9e14c Compare April 13, 2023 20:55
@fornesarturo
Copy link
Contributor Author

@cutterbl added story for the new prop and updated the API mdx to include it as well

@cutterbl
Copy link
Collaborator

@fornesarturo Thanks. I'll try to load this up and take a look over the weekend maybe.

@fornesarturo
Copy link
Contributor Author

Anything missing to have this merged?

@cutterbl
Copy link
Collaborator

@fornesarturo Finally got a chance to look at this, this morning. With an allDayMaxRows set to 2 I only saw a single event and the 'show more' button. Setting the value to null did not change the behavior. Removing the prop gave me three events, and using the prop set to 3 showed me the events. Also, the show more didn't do anything. While it did add the physical button to the display, it did not do any of the typical behaviors of the button (either the popup when using the popup prop, or the redirect without).

@cutterbl
Copy link
Collaborator

cutterbl commented Apr 19, 2023

@fornesarturo A deeper look at the failing 'show more' functionality shows that the onShowMore prop isn't getting passed in to the TimeGrid in the Day, Week or WorkWeek components, so that prop isn't available to call. In our next version we'll handle these things with context, but for now these props are manually passed down the component tree. You should also take a close look at the handleShowMore in the DateContentRow component, to see exactly what gets passed to this method, and in what order. I don't think the notify() is going to work here.

@fornesarturo
Copy link
Contributor Author

fornesarturo commented Apr 19, 2023

@cutterbl I see that first part as the usual behavior for the maxRows prop in the DateContentRow component and the getSlotMetrics function where maxRows includes the show more button row, I could make it so when passing the prop down to DateContentRow it is handled as allDayMaxRows - 1, maybe change the name of the prop to allDayMaxEventRows to make it clear, or just keep it as is. For null I can add the if else with Infinity, to handle null as undefined.

On an implementation using react-big-calendar with these changes I am getting a call to the onShowMore function, since I'm working off of that call it worked for my case. I see that what the Month view currently does is it defines the overlay there, it's not something central, I could add the functionality to TimeGrid following Month view's approach, per the commits that seems like an intentional exception:

handleShowMore = (events, date, cell, slot, target) => {
    const {
      popup,
      onDrillDown,
      onShowMore,
      getDrilldownView,
      doShowMoreDrillDown,
    } = this.props
    //cancel any pending selections so only the event click goes through.
    this.clearSelection()

    if (popup) {
      let position = getPosition(cell, this.containerRef.current)

      this.setState({
        overlay: { date, events, position, target },
      })
    } else if (doShowMoreDrillDown) {
      notify(onDrillDown, [date, getDrilldownView(date) || views.DAY])
    }

    notify(onShowMore, [events, date, slot])
  }

I see the differences in notifying the onShowMore prop here where it sends the parameters as an array, so that I can change quickly.

Let me know your thoughts.

@fornesarturo
Copy link
Contributor Author

Result of adding the Month view code to TimeGrid:

image

@fornesarturo
Copy link
Contributor Author

fornesarturo commented Apr 19, 2023

@cutterbl Added changes to handle overlay and update the onShowMore prop notify call. Also I updated the propTypes for Day and Week to reflect the props expected in TimeGrid that are drilled down from Calendar. Waiting on any comment to change handling of the limited rows in allDayMaxRows (subtract one before sending to maxRows to not include the Show more button row in the count) or keep as is (include Show more button row in the count).

@cutterbl
Copy link
Collaborator

cutterbl commented May 3, 2023

@fornesarturo I loaded your changes and brought up storybook. Still not getting the popup with the popup prop in the allDayMaxRows story...

@cutterbl
Copy link
Collaborator

cutterbl commented May 3, 2023

@fornesarturo Still not getting the popup, but I do see multiple console warnings

Warning: Failed prop type: The prop min is marked as required in Week, but its value is undefined.
Warning: Failed prop type: The prop max is marked as required in Week, but its value is undefined.
Warning: Failed prop type: The prop scrollToTime is marked as required in Week, but its value is undefined.

@fornesarturo fornesarturo force-pushed the add-show-more-in-week-view branch from 882b61f to f26b0ce Compare May 4, 2023 03:04
@fornesarturo
Copy link
Contributor Author

fornesarturo commented May 4, 2023

@cutterbl Hi, just updated to correctly display the popup, and fixed propTypes to make sense with settings across the repo.

@fornesarturo fornesarturo force-pushed the add-show-more-in-week-view branch from f26b0ce to 0aca381 Compare May 4, 2023 03:20
@cutterbl
Copy link
Collaborator

cutterbl commented May 4, 2023

@fornesarturo This is better. Now I get the popup, and with no popup I get the click through to 'Day' view. Now the only thing I see is the row count. With an allDayMaxRows: 2 I still only see one event and the +4 more link, where (by the documentation) I would expect to see two events and +3 more.

@fornesarturo fornesarturo force-pushed the add-show-more-in-week-view branch from 0aca381 to 82d29e2 Compare May 8, 2023 17:27
@fornesarturo
Copy link
Contributor Author

@cutterbl row count is fixed now and displays the number of rows for events specified.

Arturo Fornes added 2 commits June 2, 2023 11:44
* Add support for displaying Show more text in TimeGrid views via allDayMaxRows prop.
* Prevent Background cells from capturing click on show more button.
…for TimeGrid views

* Add overlay logic in TimeGrid.
* Set min-width in TimeGrid to prevent UI distortion in Day view.
* Change handleShowMore parameters and arguments for notify call.
* Change allDayMaxRows prop to reflect only event rows, not include
  the show more button row.
* Prevent empty level in slotMetrics when showMore button is present.
@fornesarturo fornesarturo force-pushed the add-show-more-in-week-view branch from 82d29e2 to 0dbe34d Compare June 2, 2023 17:47
@cutterbl cutterbl merged commit 36871bf into jquense:master Jun 2, 2023
@github-actions
Copy link

github-actions bot commented Jun 2, 2023

🎉 This PR is included in version 1.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants