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

First draft useScreenOrientation #325

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

firstchair
Copy link

No description provided.

defaultValue = false,
): boolean {
defaultValue?: boolean,
): boolean | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

in what scenario would you want undefined to be returned ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be helpful for SSR, and only on client side return a boolean 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I wonder what you would do with this then... not render something at all?
You always get a hydration error then? And this value on the client should only be used after the first render?

Comment on lines +10 to +11
isLandscape: boolean | undefined;
isPortrait: boolean | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

These are mutually exclusive, so why not return 'landscape' | 'portrait'?

@leroykorterink
Copy link
Collaborator

I don't know if it's helpful to add this as a hook because the only thing it does is provide a default value for the useMediaQuery hook. Instead we could add the landscape media query as a constant to the useMediaQuery hook file and export it and write some additional documentation.

That'll mean that you can get the same behaviour with something like this:

import { useMediaQuery, landscapeOrientationMediaQuery  } from '@mediamonks/react-kit`
const isLandscape = useMediaQuery(landscapeOrientationMediaQuery)

instead of

import { useScreenOrientation } from '@mediamonks/react-kit`
const isLandscape = useScreenOrientation()

Additionally, browsers have a not super well supported ScreenOrientation api that could be a potential naming conflict.

@ThaNarie ThaNarie requested a review from Copilot November 19, 2024 16:36
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 suggestion.

Files not reviewed (2)
  • src/hooks/useMediaQuery/useMediaQuery.mdx: Language not supported
  • src/hooks/useScreenOrientation/useScreenOrientation.mdx: Language not supported
Comments skipped due to low confidence (2)

src/hooks/useScreenOrientation/useScreenOrientation.ts:17

  • Add a test case to cover the scenario where isLandscape is undefined to ensure the behavior is as expected.
isPortrait: !isLandscape,

src/hooks/useMediaQuery/useMediaQuery.ts:57

  • The return type of useMediaQuery has been changed to boolean | undefined. This might introduce unintended behavior if the calling code does not handle undefined values. Ensure that all usages of useMediaQuery are updated to handle undefined values appropriately.
): boolean | undefined {

src/hooks/useScreenOrientation/useScreenOrientation.ts Outdated Show resolved Hide resolved
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