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

トゥートの並び順をフリップするボタンを作成 #81

Merged
merged 5 commits into from
Jan 11, 2020
Merged

トゥートの並び順をフリップするボタンを作成 #81

merged 5 commits into from
Jan 11, 2020

Conversation

paihu
Copy link
Contributor

@paihu paihu commented Jan 11, 2020

related issue: #77

割と強引に入れ替えているので、リストが長いと時間かかるかもしれません。

ボタンの位置は適当なので不満があればいい感じに入れ替えてもらえると良さそうです。

@paihu paihu marked this pull request as ready for review January 11, 2020 13:56
@yume-yu
Copy link
Collaborator

yume-yu commented Jan 11, 2020

@paihu すみません、jsに自信がないので確認したいのですが、これって構造としてカードのdiv自体の順番を変えてしまう感じの動きをしますか?
あと、一度ひっくり返したときってどこに追加されますか?

js/index.js Outdated
card_nodes.push(cards.firstChild);
cards.removeChild(cards.firstChild);
}
if (card_nodes.length == 0) return;
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

Choose a reason for hiding this comment

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

あと、そもそもですが、card_nodes.lengthで比較するよりは、!cards || !cards.hasChildNodes()の方がわかりやすいかもしれません

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cardsに空のtext node があることが分かったのでタイミングによって cards.childNodesがあっても card_nodes.length が0となることがありますのでここで確認しておくことに意味はありそうです。

Copy link
Contributor

Choose a reason for hiding this comment

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

あー、意味がわかりました、childNodesがあっても飛ばすからということですね。最新の奴見て納得しました。

@KEINOS
Copy link
Member

KEINOS commented Jan 11, 2020

PR のプレビュー: https://deploy-preview-81--zen-edison-40804e.netlify.com/
アイコンの長いまとめ: https://git.io/JvfOk

で試しました。

初回読み込みは時間がかかるもの、「Flip」押下でサクッと入れ替わりました!!

ただ、最新の master にアップデートされていないようで、パーマリンクが以前のドメインになってしまいます。

スクリーンショット 2020-01-11 23 11 37

現在は、作成時のドメインを見てダイナミックに変わるので、アップデートすると直ると思います。

コンフリクトしなければいいのですが 🤞

@sasanquaneuf
Copy link
Contributor

パーマリンクの件は、今回の差分には含まれていないので、マージすると正しく動くと思います。
(最新のmasterに合わせてrebaseしてからPR出す運用もありますが、徹底するのはちょっと大変そう...ただ、rebaseした方がコミットグラフはきれいになります)

js/index.js Outdated Show resolved Hide resolved
@blhsrwznrghfzpr blhsrwznrghfzpr added the enhancement New feature or request label Jan 11, 2020
Copy link
Collaborator

@blhsrwznrghfzpr blhsrwznrghfzpr left a comment

Choose a reason for hiding this comment

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

LGTM

@sasanquaneuf
Copy link
Contributor

ねんのためLGTM(待ちになってたらすみません)

@KEINOS
Copy link
Member

KEINOS commented Jan 11, 2020

@yume-yu マジ卍して 👌 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants