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

<Button />微調整 #28

Merged
merged 12 commits into from
May 18, 2020
Merged

<Button />微調整 #28

merged 12 commits into from
May 18, 2020

Conversation

youchann
Copy link
Contributor

  • aタグとしても機能するように
  • box-shadowを調整

@youchann youchann self-assigned this May 18, 2020
Comment on lines 107 to 109
as={href ? "a" : "button"}
href={href}
{...(href ? {} : rest)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

aタグにすると...restは入れないようにしています。(型がうまく通らない...)

なんかもっと良い方法ありそうなきが...

Copy link
Contributor

Choose a reason for hiding this comment

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

next.jsのコードなのでちょっと違うけどinlineじゃなくて先に定義した方がいいと思う!

const isLink = href != null
  let props: any

  if (isLink && disabled) {
    props = { "aria-disabled": disabled }
  } else if (isLink) {
    props = { href }
  } else {
    props = { type, disabled }
  }

  return isLink ? (
    <Link href={href!} passHref>
      <StyledBaseButton {...props} {...rest} />
    </Link>
  ) : (
    <StyledBaseButton {...props} {...rest} />
  )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

あ、prop先に定義します!!

aタグでbuttonタグを囲う感じですかね...??

Copy link
Contributor

Choose a reason for hiding this comment

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

あ、そこがnextっぽいところ
ingred-uiではasで大丈夫!

@youchann youchann requested a review from kinokoruumu May 18, 2020 04:07
Comment on lines 104 to 110
const restProps = href ? {} : rest;

return (
<Styled.ButtonContainer
as={href ? "a" : "button"}
href={href}
{...(href ? {} : rest)}
{...restProps}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kinokoruumu
restに関しては先に定義しました。

書いてて思ったのですが、以前のcommentで言っていたことはこのような書き方ではなく「props全体を先に定義させる」みたいな意図でしたかね...?

Comment on lines 104 to 119
let props: Styled.ContainerProps & { as?: "a" | "button" } = {
inline,
horizontalPadding: size === "small" ? `10px` : `${theme.spacing * 2}px`,
normal: { ...colorStyle.normal },
hover: { ...colorStyle.hover },
active: { ...colorStyle.active },
fontWeight: color === "cancel" ? "normal" : "bold",
fontSize: size === "small" ? `${fontSize["xs"]}px` : `${fontSize["md"]}px`,
height: buttonSize[size].height,
minWidth: buttonSize[size].minWidth,
}

if (isLink) {
props.as = "a";
props.href = href;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kinokoruumu
hrefが存在するときだけ、ashrefをpropsに入れるようにしました!!

size === "small" ? `10px` : `${theme.spacing * 2}px`;
const isLink = !!href;

let props: Styled.ContainerProps & { as?: "a" | "button" } = {
Copy link
Contributor

Choose a reason for hiding this comment

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

asに"button"が入ってなさそう

Copy link
Contributor Author

Choose a reason for hiding this comment

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

デフォルトが`buttonなので、入れなくても良いかと思ってました。

とりあえず入れときました!!

Copy link
Contributor

Choose a reason for hiding this comment

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

それなら型定義にもなくていいんじゃないかな?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BaseButton = styled.buttonなのでasをpropsに含めない場合は勝手に<button />タグになるという意味での

デフォルトが`button

でした!!

& { as?: "a" | "button" }の部分を消してしまうと、�props.as = "a";の部分で型エラーになってしまいます。

Copy link
Contributor

Choose a reason for hiding this comment

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

そうそう
さっきのvcはそれを見越してのpropsはanyでもいいよ〜だったw
で、今のはasにbuttonを入れることがないのであれば | "button" が不要なのでは?という意味だったw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なんと...w:bow:

let props: anyにして、asの型定義を外しました!!

Copy link
Contributor

Choose a reason for hiding this comment

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

ごめん、なんども往復して申し訳ないんだけど、
let propsはhrefとasだけでいいと思う

ContainerPropsの内容も含めたいのであれば量が多いので型定義はあった方がいいかと
曖昧でごめん。。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

d991892
こんな感じの実装でどうでしょう!?

Copy link
Contributor

@kinokoruumu kinokoruumu left a comment

Choose a reason for hiding this comment

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

LGTMです!

@youchann youchann merged commit 6740697 into master May 18, 2020
@youchann youchann deleted the tweak-button branch May 18, 2020 08:44
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.

None yet

2 participants