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

fix: render resources under each day in header column #2609

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fs-nathan
Copy link

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

The best way to propose a feature is to open an issue first and discuss your ideas there before implementing them.

Always follow the contribution guidelines when submitting a pull request.

@cutterbl
Copy link
Collaborator

While this initially appears really good, it completely changes the current default functionality, making it a breaking change. This would be better served by a new prop, with refactor to use as an alternative view, providing both the default (current) and new.

@almeidapaulooliveira
Copy link

To make it simple and be able to merge, what if we have a prop (exposed in Calendar) to render the week on default and this? Will avoid the work of refactoring to an alternative view and quickly release the new feature.

Wdyt?

@cutterbl
Copy link
Collaborator

@almeidapaulooliveira It's not about having the week on default (that's already a given). It's the difference between grouping days under resources (current display) as opposed to resources under days. I agree that a top level prop (resourceGroupingLayout?) allowing to switch is good, but that would require conditional logic in TimeGrid for both the grouping logic, and application of flexbox wrapper components.

@almeidapaulooliveira
Copy link

@cutterbl, you're right, that's what I meant. Sorry if I didn't express it well.

I forked @fs-nathan's repo and I'll try implementing this conditional logic. If it's successful, I'll reach out to you (in a PR) for a review.

Hopefully, we can merge it soon.

@almeidapaulooliveira
Copy link

@cutterbl I created a new PR, that implements what we discussed. Looking forward for your review.

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.

3 participants