acl-services / paprika

🌶 A robust + accessible UI component library for React applications by Galvanize.
MIT License
54 stars 9 forks source link

UXD-1888 update ResizeDetector and useDeboucedCallback #1197

Closed AndreyChernykh closed 2 years ago

AndreyChernykh commented 2 years ago

Purpose 🚀

Migrate the package to TS

Updates 📦

Before:

import { useDimensions, useBreakpoints } from "../src";
function MyComponent() {
  const { width, height } = useDimensions();
  const { size } = useBreakpoints();

After:

import { useResizeDetectorContext } from "../src";
function MyComponent() {
  const { width, height, breakpointSize } = useResizeDetectorContext();

Storybook 📕

http://storybooks.highbond-s3.com/paprika/your-branch-name

Screenshots 📸

optional but highly recommended

References 🔗

UXD-1888

changeset-bot[bot] commented 2 years ago

🦋 Changeset detected

Latest commit: 765599704a8938be06bc7b039d1257f767d15d50

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages | Name | Type | | ------------------------ | ----- | | @paprika/resize-detector | Major | | @paprika/helpers | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

allison-c commented 2 years ago

Just curious why changing isFullWidth default value to false?

AndreyChernykh commented 2 years ago

@allison-c I believe that having boolean values to have a default of false is more conventional. If the default value is false then:

<MyComponent isFlag /> // isFlag === true
<MyComponent /> // isFlag === false

However, if the flag is true by default, then we need to do this:

<MyComponent /> // isFlag === true
<MyComponent isFlag={false} /> // isFlag === false

If we really want to have true by default, then one approach is to re-phrase the prop. E.g. instead of isVisible: true -> isHidden: false But in this particular case we have 2 flags that are related: isFullWidth and isFullHeight. Rephrasing one them would be hard and will probably lead to an inconsistent naming. And having them with one true by default and the other false by default is kinda weird 🤷‍♂️

@mikrotron cc

allison-c commented 2 years ago

Looks good to me overall, but I'm not very familiar about the breakpoint use cases, could you take a look as well please @mikrotron

mikrotron commented 2 years ago

@allison-c I believe that having boolean values to have a default of false is more conventional. If the default value is false then:

<MyComponent isFlag /> // isFlag === true
<MyComponent /> // isFlag === false

However, if the flag is true by default, then we need to do this:

<MyComponent /> // isFlag === true
<MyComponent isFlag={false} /> // isFlag === false

If we really want to have true by default, then one approach is to re-phrase the prop. E.g. instead of isVisible: true -> isHidden: false But in this particular case we have 2 flags that are related: isFullWidth and isFullHeight. Rephrasing one them would be hard and will probably lead to an inconsistent naming. And having them with one true by default and the other false by default is kinda weird 🤷‍♂️

@mikrotron cc

I partially agree – I also PREFER that boolean props are false by default. But we do have exceptions in our system, and if the rephrased name would be harder to understand, then I'd prefer to use a true default value. isFullWidth is easy to understand and has a precedent in other components, so I think we should keep that name.

But I also like that this component behaves like a block level <div> element by default. I think that's a better, more intuitive default behaviour than an inline-block / <span>. I think it's also a more common use case.

It's not perfect, I don't really like that isFullWidth=true and isFullHeight=false by default, and would prefer that booleans are false be default, but I think clear naming, and sensible default behaviour is more important.

Are you convinced?

AndreyChernykh commented 2 years ago

OK, I'll change isFullWidth back to true by default