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

Feat/5569 5603 without proxy accordion #3599

Merged
merged 52 commits into from
Apr 12, 2021

Conversation

stevenfukase
Copy link
Contributor

Without Proxy設定画面内にアコーディオンを追加しました。
【英語】
Eng

【日本語】
Jpn

【中国語】
Chi

@stevenfukase stevenfukase requested a review from itizawa April 8, 2021 04:42
Copy link
Contributor

@itizawa itizawa left a comment

Choose a reason for hiding this comment

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

コメントしました。確認お願いします

@stevenfukase stevenfukase changed the title Feat/5569 without proxy accordion Feat/5569 5603 without proxy accordion Apr 8, 2021
Copy link
Contributor

@itizawa itizawa left a comment

Choose a reason for hiding this comment

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

コメントしました。確認お願いします

Comment on lines +7 to +18
const [openAccordionIndexes, setOpenAccordionIndexes] = useState(new Set());

const onToggleAccordionHandler = (i) => {
const accordionIndexes = new Set(openAccordionIndexes);
if (accordionIndexes.has(i)) {
accordionIndexes.delete(i);
}
else {
accordionIndexes.add(i);
}
setOpenAccordionIndexes(accordionIndexes);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

参考:

toggleCheckbox(e) {
const { target } = e;
const { name, checked } = target;
this.setState((prevState) => {
const selectedCollections = new Set(prevState.selectedCollections);
if (checked) {
selectedCollections.add(name);
}
else {
selectedCollections.delete(name);
}
return { selectedCollections };
});
}

Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど、state がすでに Set オブジェクトの場合毎回 new Set で初期化するのは無駄だと思いましたがどうですかね...

Copy link
Contributor

@itizawa itizawa Apr 12, 2021

Choose a reason for hiding this comment

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

最近知ったんですけど、関数内で現在の state を参照して操作するのではなく prevState を操作した方が正しそう?
今回は直さなくていいですが

 const onToggleAccordionHandler = (i) => {
  setOpenAccordionIndexes((prevOpenAccordionIndexes)=>{
  const accordionIndexes = prevOpenAccordionIndexes
   if (accordionIndexes.has(i)) {
      accordionIndexes.delete(i);
    }
    else {
      accordionIndexes.add(i);
    }
   return accordionIndexes;
  }
}

参考:https://zenn.dev/stin/articles/use-appropriate-api#%E7%B5%90%E8%AB%96

Comment on lines +10 to +18
const accordionIndexes = new Set(openAccordionIndexes);
if (accordionIndexes.has(i)) {
accordionIndexes.delete(i);
}
else {
accordionIndexes.add(i);
}
setOpenAccordionIndexes(accordionIndexes);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

これだと毎回 Set オブジェクトを生成し直していますね。

Comment on lines +7 to +18
const [openAccordionIndexes, setOpenAccordionIndexes] = useState(new Set());

const onToggleAccordionHandler = (i) => {
const accordionIndexes = new Set(openAccordionIndexes);
if (accordionIndexes.has(i)) {
accordionIndexes.delete(i);
}
else {
accordionIndexes.add(i);
}
setOpenAccordionIndexes(accordionIndexes);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [openAccordionIndexes, setOpenAccordionIndexes] = useState(new Set());
const onToggleAccordionHandler = (i) => {
const accordionIndexes = new Set(openAccordionIndexes);
if (accordionIndexes.has(i)) {
accordionIndexes.delete(i);
}
else {
accordionIndexes.add(i);
}
setOpenAccordionIndexes(accordionIndexes);
};
const [openAccordionIndexes, setOpenAccordionIndexes] = useState(new Set([]));
const onToggleAccordionHandler = (i) => {
if (accordionIndexes.has(i)) {
setOpenAccordionIndexes(accordionIndexes.delete(i));
}
else {
setOpenAccordionIndexes(accordionIndexes.add(i));
}
};

これで動くような?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちら、試しましたが動きませんでした。

  const [openAccordionIndexes, setOpenAccordionIndexes] = useState(new Set([]));

  const onToggleAccordionHandler = (i) => {
    if (openAccordionIndexes.has(i)) {
      setOpenAccordionIndexes(openAccordionIndexes.delete(i));
    }
    else {
      setOpenAccordionIndexes(openAccordionIndexes.add(i));
    }
  };

Comment on lines +7 to +18
const [openAccordionIndexes, setOpenAccordionIndexes] = useState(new Set());

const onToggleAccordionHandler = (i) => {
const accordionIndexes = new Set(openAccordionIndexes);
if (accordionIndexes.has(i)) {
accordionIndexes.delete(i);
}
else {
accordionIndexes.add(i);
}
setOpenAccordionIndexes(accordionIndexes);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど、state がすでに Set オブジェクトの場合毎回 new Set で初期化するのは無駄だと思いましたがどうですかね...

Comment on lines +7 to +18
const [openAccordionIndexes, setOpenAccordionIndexes] = useState(new Set());

const onToggleAccordionHandler = (i) => {
const accordionIndexes = new Set(openAccordionIndexes);
if (accordionIndexes.has(i)) {
accordionIndexes.delete(i);
}
else {
accordionIndexes.add(i);
}
setOpenAccordionIndexes(accordionIndexes);
};
Copy link
Contributor

@itizawa itizawa Apr 12, 2021

Choose a reason for hiding this comment

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

最近知ったんですけど、関数内で現在の state を参照して操作するのではなく prevState を操作した方が正しそう?
今回は直さなくていいですが

 const onToggleAccordionHandler = (i) => {
  setOpenAccordionIndexes((prevOpenAccordionIndexes)=>{
  const accordionIndexes = prevOpenAccordionIndexes
   if (accordionIndexes.has(i)) {
      accordionIndexes.delete(i);
    }
    else {
      accordionIndexes.add(i);
    }
   return accordionIndexes;
  }
}

参考:https://zenn.dev/stin/articles/use-appropriate-api#%E7%B5%90%E8%AB%96

Comment on lines +9 to +18
const onToggleAccordionHandler = (i) => {
const accordionIndexes = new Set(openAccordionIndexes);
if (accordionIndexes.has(i)) {
accordionIndexes.delete(i);
}
else {
accordionIndexes.add(i);
}
setOpenAccordionIndexes(accordionIndexes);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

今後、完了した step によって accordion の開閉をコントロールするのだとしたら index ではなく key を保存した方が良さそうな気がしました。
必要であれば後ほどのタスクで対応するようにお願いします

@stevenfukase stevenfukase merged commit 59f807e into feat/growi-bot Apr 12, 2021
@stevenfukase stevenfukase deleted the feat/5569-without-proxy-accordion branch April 12, 2021 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants