[Feature]: Add emotion position function types

[yoonminsang]

Package Scope

• [x] Add to an existing package
• [ ] New package

Package name: react/emotion-utils

Overview

I want to like code like this position({top:10,bottom:8}) But position function did not support this type.

Describe the solution you'd like

I want to add this code.

export function position(coordinates: Coordinates): SerializedStyles;`

  const [top, right, bottom, left] = (() => {
    // position(coordinates)
    if (typeof positionOrTopOrCoordinates === 'object') {
      return [
        positionOrTopOrCoordinates.top,
        positionOrTopOrCoordinates.right,
        positionOrTopOrCoordinates.bottom,
        positionOrTopOrCoordinates.left,
      ];
    }

and test code

  it('position({top: 0, left: 0})', () => {
    const { getByTestId } = render(<div data-testid="test" css={position({ top: 0, left: 0 })} />);

    const el = getByTestId('test');

    expect(el).toBeInTheDocument();

    expect(el).toHaveStyleRule('top', '0');
    expect(el).not.toHaveStyleRule('bottom', '0');
    expect(el).not.toHaveStyleRule('right', '0');
    expect(el).toHaveStyleRule('left', '0');
  });

Additional context

I think toss team may not agree my opinion. So I open an issue, not pr. Thank you

[hoseungme]

You can use position() utility as you want, by specifying the value of position property:

<div css={position('absolute', { top: 10, bottom: 8 })} />

Do you feel the same difficulty when you use position() like above?

[yoonminsang]

In general, but there are exceptions.

I am making Tooltip component. This is my code. I don't want duplicate absolute. But it may be my own thoughts.

  const getArrowPositions = useCallback(() => {
    switch (position) {
      case 'top-start':
        return css`
          &::after {
            ${theme.position({ bottom: 0, left: ARROW_WIDTH })}
            margin-bottom: -${ARROW_WIDTH}px;
            ${topBorderColor}
          }
        `;
      case 'top':
        return css`
          &::after {
            ${theme.position({ bottom: 0, left: '50%' })}
            margin-left: -${ARROW_HEIGHT}px;
            margin-bottom: -${ARROW_WIDTH}px;
            ${topBorderColor}
          }
        `;
    // skip
  },[]);

      <div
        css={css`
          // skip
          ${arrow && [
            css`
              &::after {
                content: '';
                position: absolute;
                border-style: solid;
                border-width: ${ARROW_HEIGHT}px;
              }
            `,
            getArrowPositions(),
          ]}
        `}
      >
        {title}
      </div>

If my explanation wasn't enough, you can show this link. https://github.com/yoonminsang/play-ground/blob/develop/packages/common-components/src/Tooltip/Tooltip.tsx#L122

[hoseungme]

In your case it looks needed. @raon0211 @changyoungoh Any opinion?

[raon0211]

In my opinion, since top, left, bottom, and right have no effect when position is not given, they should always go together.

So in the source code above, every call to position should include 'absolute' in the first argument.

  const getArrowPositions = useCallback(() => {
    switch (position) {
      case 'top-start':
        return css`
          &::after {
            ${theme.position('absolute', { bottom: 0, left: ARROW_WIDTH })}
            margin-bottom: -${ARROW_WIDTH}px;
            ${topBorderColor}
          }
        `;
      case 'top':
        return css`
          &::after {
            ${theme.position('absolute', { bottom: 0, left: '50%' })}
            margin-left: -${ARROW_HEIGHT}px;
            margin-bottom: -${ARROW_WIDTH}px;
            ${topBorderColor}
          }
        `;
    // skip
  },[]);

Otherwise, we might forget the implicit connection between position and top in the long run. Accidentally deleting position: absolute in the component will result in broken code.

[hoseungme]

I agree and understand what you are concerned about, but I think the important point of this issue is that we have already allowed using it without giving position:

css`
  position: fixed;
  ${position(0, 0, 0, 0)}
`

So, in this case, @yoonminsang and others can be confused about why all coordinates should be passed and I think this issue also derived from it.

[hoseungme]

But I think we don't have to support it at the library level though.

Because:

• We have been using this position utility for a long time but we couldn't see any needs like this issue.
• I agree the comment by @raon0211 :

  • we might forget the implicit connection between position and top in the long run. Accidentally deleting position: absolute in the component will result in broken code.

Therefore my suggestion is, to add the simple custom function below to your project and use it:

// utils/absolute.ts
import { position } from '@toss/emotion-utils';

export function absolute(coordinates: Coordinates) {
  return position('absolute', coordinates)
}

// components/Tooltip.tsx
import { absolute } from "utils/absolute";

switch (position) {
  case 'top-start':
    return absolute({ bottom: 0, left: ARROW_WIDTH });
  case 'top':
    return absolute({ bottom: 0, left: '50%' });
  /* ... */
}

@yoonminsang How about this?

[raon0211]

Yes, I agree in @HoseungJang ’s opinion. And I think we should deprecated the API where position value is omitted.

[yoonminsang]

Thank you for comments. I agree. If you think you need an absolute, fixed, sticky function, I will open pr. Otherwise, I will close this issue.

[raon0211]

I think it will be nice to have absolute, fixed, sticky functions in our emotion utils. cc. @HoseungJang

[hoseungme]

It also looks great to me. I would like to add those new utils to position's property. What do you think? @raon0211 @yoonminsang

import { position } from '@toss/emotion-utils';

position.absolute({ top, left });
position.fixed({ top, left, right, bottom });
/* ... */

[raon0211]

I agree.

[yoonminsang]

I agree too. It is different from the content of the issue I posted. Shall I write the code? Or will @HoseungJang write it?

[hoseungme]

@yoonminsang I hope you to write the code. Could you help us?

[yoonminsang]

Of course. Thank you~ Maybe I can upload the work tomorrow or day after tomorrow night.

[hoseungme]

Thanks. Take your time!

[yoonminsang]

We have to make a few choices. @raon0211 @HoseungJang

1. Support only absolute, fixed, sticky vs Support 'static', 'relative', 'absolute', 'fixed', 'sticky', '-webkit-sticky'
2. Support only position.absolute({top:0,bottom:0}) vs Support position.absolute(0,0,0,0)

I'll upload whatever you want.

[hoseungme]

1. I think we can just support absolute, fixed and sticky. Because other values are not used with top, left, right and bottom in general.
2. I think it would be great to support the both interfaces.