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

feat(useGrid): fallback size when containerWidth is 0 #38

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

feat(useGrid): fallback size when containerWidth is 0 #38

wants to merge 1 commit into from
+7 −1

Conversation

luizcieslak
Copy link

Closes #37

I tried to write a unit test for useGrid but it fails since the returned ref must be place somewhere:

import { renderHook } from '@testing-library/react-hooks'
import { enGB } from 'date-fns/locale'
import useGrid from '../src/useGrid'

describe('useGrid', () => {
  test('should have cellHeight greated than 6px', () => {
    const { result } = renderHook(() => useGrid({ locale: enGB, onMonthChange: () => {}, transitionDuration: 300 }))

    expect(result.current.cellHeight).toBeGreaterThan(6)
  })
})

I also tried toHaveAttribute from jest-dom with no success.

It seems to solve the problem tho, I tested manually across different devices.

If you have a suggestion regarding the test, I can update the PR before merging.

@luizcieslak luizcieslak mentioned this pull request Jul 8, 2020
Open
@hernansartorio
Copy link
Owner

hernansartorio commented Jul 10, 2020 •
edited

Thank you for taking the time for this!

I was just trying it locally, it works well for the default size, but if you change the container size to something else then the aspect ratio gets distorted (notice the circles on hover):

image

image

So I'm not sure if setting a fixed fallback size is actually the proper solution 🤔

@abriginets
Copy link

abriginets commented Jul 16, 2020 •
edited

@hernansartorio hello. I started with your calendar today and it's absolutelly great with it's minimalistic and intuitive API, but unfortunatelly I faced exact same issue while trying to wrap it with popperjs' tooltips. I think the best solution here is to rewrite days to flexbox rows (instead of line-breaking technique with calc(100% / 7)) and make days having a fixed height and width, which will make calendar behavior more predictible. Trying to acomplish absolute responsivenes will guarantee you something will broke one day, so as the developer and maintainer of this package you should provide a simple static layout and let users to deal with responsiveness by themselves since they know their consumers better.

I'm sorry if this reads like I'm trying to teach you on how to do things. Just collecting my thoughts about this PR :)

UPD: just got an idea. Why don't you keep DatePicker responsive, but refactor standalone Calendar to have static layout :)

@hernansartorio
Copy link
Owner

@JamesJGoodwin hey, thanks for the input. Well, needing a responsive date picker was one of the main reasons I built it so making it fixed by default would defeat that purpose.

I still need to think about this more and play with potential solutions, but I'm beginning to think that adding a width prop could be a good alternative.

In any case, this is easily solvable on your side as I mentioned here at the end.

vibin-230 pushed a commit to vibin-230/react-nice-dates that referenced this pull request Feb 2, 2021
@kumargugloth
Merge pull request hernansartorio#38 from kumargugloth/develop
39ba869 
fixes of repeat booking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Styles not being applied when component is inside a collapsible element
3 participants
@luizcieslak @hernansartorio @abriginets