Skip to content

Lesson31-2: Added the ability to change the speed of the menu #51

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

Merged
merged 6 commits into from
Mar 25, 2023

Conversation

haru-programming
Copy link
Owner

@haru-programming haru-programming commented Mar 22, 2023

Issue No.31

  • 課題30で作ったドロワーメニューはオプションで いろいろな機能を渡せるようにしてください。

今回実装した仕様

  • もしくはスライドスピードとモーションを変更できる等 引数に値を渡したら切り替えられるようにしてください
     => スライドスピードのみ対応することとしました🙇‍♀️

実装済みの仕様

  • 例えば、 左から出てくるように実装してもらいましたが それを右から出てくるように設定できる
  • 左右どちらか選べるやつは必須でそのほかは余力があればでokです
    -> 左右どちらか選べるのみ実装
    {direct: "left"} // or right

CodeSandBox

forkに失敗する場合のCodeSandbox(あらかじめfork ver.)

@haru-programming haru-programming self-assigned this Mar 22, 2023
@codesandbox
Copy link

codesandbox bot commented Mar 22, 2023

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

Comment on lines 154 to 155
transition: transform 0.3s ease-in-out;
will-change: transform;
transition: transform ease-in-out;
Copy link
Owner Author

@haru-programming haru-programming Mar 22, 2023

Choose a reason for hiding this comment

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

CSS

  • I removed transition-duration. -> JavaScript to determine and add.
  • Added will-change to make the animation smooth.

Comment on lines 46 to 56
const changeDirect = (ref, direct) => {
const changeDirect = (menu, direct) => {
switch(direct){
case "right":
ref.classList.add("right");
menu.classList.add("right");
break;
case "left":
default:
ref.classList.add("left");
menu.classList.add("left");
break;
}
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

Correction of previous PR

I have changed the argument name to menu based on your previous comment.

ここのrefは簡単に書いてしまいましたが、関数名がMenuとして含まれているのでもっと具体的でいいと思います。前回書いていたものでも構わないです
#50 (comment)

break;
}
}

const option = { direct: "left" }; //left or right
const convertMillisecondsToSeconds = speed => `${speed / 1000}s`;
Copy link
Owner Author

Choose a reason for hiding this comment

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

convert ms(ミリ秒) to s(秒)

  • Since milliseconds were also used in libraries such as swiper and splide, milliseconds were adopted as an option.
  • I created this function because we thought it would be more common to convert the number of seconds into the number of seconds when setting transition-duration.

Comment on lines 60 to 73
const changeSpeed = (menu, speed) => {
const defaultSpeed = "0.4s";
const judgedSpeed = typeof speed === "number"? convertMillisecondsToSeconds(speed): defaultSpeed;
menu.style.transitionDuration = judgedSpeed;
}

const option = {
direct: "left", //left or right
speed: 400 // number(ミリ秒)
};

const initMenu = (menu, option = {}) => {
changeDirect(menu, option?.direct);
changeSpeed(menu, option?.speed);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Add option of speed

1. Add milliseconds to option.

2. In the changeSpeed function
- argument is typeof number -> run convertMillisecondsToSeconds()
- type of others -> set defaultSpeed(0.4s)
 
3. Execute the changeSpeed function in initMenu().
- const initMenu = (menu, option = {}) => {
  -> If no options are available, {} will be passed
- changeSpeed(menu, option?.speed);
  -> If there is no speed key in the options, undefined is passed

@haru-programming
Copy link
Owner Author

haru-programming commented Mar 22, 2023

動作確認について

js/drawer-menu.js 内のoption値を変更していただきたいです🙇‍♀️

const option = { 
    direct: "left", //left or right
    speed: 400 // number(ミリ秒)
};

① speedのミリ秒の数字を変更する

-> ドロワーメニューの開閉スピードが変わる (header__navクラスにtransition-durationがセットされる)
-> 数字が小さいと速く、大きいと遅くなります。

② speedを"" や "abcdefg" など適当な文字に変更する

-> errorがthrowされ、アニメーションがつかない。(開発者にエラーを伝える)

③ speedをコメントアウトする

const option = { 
    direct: "left", //left or right
    // speed: 400 // コメントアウト
};

-> デフォルト値0.4sの開閉スピードになる。

お手数をおかけいたしますが、よろしくお願いいたします🙇‍♀️


const initMenu = (menu, option = {}) => {
changeDirect(menu, option?.direct);
changeSpeed(menu, option?.speed);
Copy link

Choose a reason for hiding this comment

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

IMO
必ずしもchangeしないと思うのでset**でいい気がしますね

Copy link
Owner Author

Choose a reason for hiding this comment

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

ご指摘ありがとうございます!
その通りだと思ったので修正いたしました。
b4627ea

changeDirect(ref, option?.direct);
const changeSpeed = (menu, speed) => {
const defaultSpeed = "0.4s";
const judgedSpeed = typeof speed === "number"? convertMillisecondsToSeconds(speed): defaultSpeed;
Copy link

@kenmori kenmori Mar 22, 2023

Choose a reason for hiding this comment

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

Q
speedをjudgeしているというよりはtypeをcheckしているのですよね?
あー、その後処理がありました。コメントしました
ref

Copy link
Owner Author

@haru-programming haru-programming Mar 23, 2023

Choose a reason for hiding this comment

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

ご指摘ありがとうございます!
formatする関数と、setする関数を分けた方がわかりやすいと解釈しました!

一度修正してみたのですが、optionに数字以外のものが渡った場合は、どのようにすべきか迷ってしまいました。
b4627ea

【現状】

optionにspeedキーがないとき(引数にundefinedが渡る時)は、デフォルト引数の400が使用されます。
option = {speed: "a"}などが誤って渡ったときは、デフォルト引数の値が使用されず、transition-durationがセットされない状態です。

【考えているコード】

下記のように実装するとundefinedにも数字以外の値にも対応できると考えたのですが、
(デフォルト引数は使用しなくなってしまいます..)
この書き方は 適していないでしょうか?

const formatSeconds = (speed) => {
    const default = 400;
    if(typeof speed !== "number"){
        convertMillisecondsToSeconds(default);
        return;
    }
    return convertMillisecondsToSeconds(speed);
}

オプション機能を使用する側にエラーを伝えてreturnする
(デフォルト値は渡さず、一旦アニメーションはつかなくなる)
ということも考えました。

const formatSeconds = (speed) => {
    if(typeof speed !== "number"){
        console.error("speedオプションには数字を入力してください");
        return;
    }
    return convertMillisecondsToSeconds(speed);
}

ご教授いただけると嬉しいです。
お忙しい中恐れ入りますが、ご確認のほどよろしくお願いいたします。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@kenmori
Discordでご教授いただきありがとうございました!
大変勉強になりました。

下記の場合に対応できるコードに修正しました。

  • undefined -> デフォルト引数 speed = 400
  • undefined, number以外 -> throw error

ご確認のほどよろしくお願いいたします🙇‍♀️

aa97871
CodeSandBox

Copy link

Choose a reason for hiding this comment

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

const formatSeconds = (speed) => {
    const default = 400;
    if(typeof speed !== "number"){
        convertMillisecondsToSeconds(default);
        return;
    }
    return convertMillisecondsToSeconds(speed);
}

↑これが考えているコードですね
質問の内容は、speedundefinedで渡ってきた場合に対応するために上のように書いたがこれだとダメかということですね
いいのですが、
defaultは必ずしもそこで定義しなくてもいいと思います
書くなら

const formatSeconds = (speed) => {
    if(typeof speed !== "number"){
        const default = 400;
        convertMillisecondsToSeconds(default);
        return;
    }
    return convertMillisecondsToSeconds(speed);
}

ただ、

speedがundefinedが渡ってくるとわかっているなら、
const formatSeconds = (speed = 400) => {
    if(typeof speed !== "number"){
        convertMillisecondsToSeconds(speed);
        return;
    }
    return convertMillisecondsToSeconds(speed);
}

こうなりますね。これのいいところは不要な定義を避けているところです。
これを生かすとまだ冗長ですね
問題はnumberundefined以外が渡ってくるとき、ですが
これは仕様的にあり得ないですが、もしそうならerrorにするか、変換するかなのですが、
全てのtypeに対応してチェックすると大変だと思うのでそこはどこまで堅牢にするかによるかと思います
全てのタイプをチェックしないのであれば、開発時にエラーをthrowして気づかせることができます

const formatSeconds = (speed = 400) => {
    if(typeof speed !== "number"){
        throw new Error(`speed expect number type. but got ${typeof speed}`)
    }
    return convertMillisecondsToSeconds(speed);
}

個人的にはちゃんとやるなら上ですが、そこまで望まないです
ここを開発時にちゃんと保証させるのがTypeScriptです。JSで堅牢な関数を作るのであればちゃんと型チェックするのもいいですが、
console.error()

はあくまでクラッシュさせたくない場合に使います。ユーザーの画面で使うこともありますが、
エラーが出続けた状態で何か使われると不正なデータが入ったりしてさらにバグ発見が困難な状態になる場合は
クラッシュさせて伝えることも大事なので、そういう時はエラーを返します


const initMenu = (ref, option = {}) => {
changeDirect(ref, option?.direct);
const changeSpeed = (menu, speed) => {
Copy link

@kenmori kenmori Mar 22, 2023

Choose a reason for hiding this comment

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

Suggested change
const changeSpeed = (menu, speed) => {
const formatSeconds = (menu, speed = 400) => {
return convertMillisecondsToSeconds(speed)
}

Copy link
Owner Author

@haru-programming haru-programming Mar 23, 2023

Choose a reason for hiding this comment

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

formatSeconds()のご提案コードありがとうございます!
上記と一緒に修正しております!
#51 (comment)

@haru-programming haru-programming changed the title feat: Added the ability to change the speed of the menu lesson31-2: Added the ability to change the speed of the menu Mar 23, 2023
@haru-programming haru-programming changed the title lesson31-2: Added the ability to change the speed of the menu Lesson31-2: Added the ability to change the speed of the menu Mar 23, 2023
@nakagawamai
Copy link

I specified the options as follows and this is how it works. Please check it.
This always happens when I set up and reload the screen. when direct was left, there was no problem.

const option = {
  direct: "right",
  speed: 1000
};
image2.mov

@haru-programming
Copy link
Owner Author

@nakagawamai
Thank you for your review!
You are right! It was also happening in my environment.

I don't know if this is the right solution, but I decided to animate by changing the left or right value instead of tranform.
I also set opacity: 1 after adding the left and right classes so that the menu movement would not be visible.

.header__nav{
    position: fixed;
    z-index : 1200;
    opacity: 0; // 追加
    top : 0;
    color: #000;
    background: #fff;
    text-align: center;
    width: 40%;
    height: 100%;
    overflow-y: scroll;
}

.header__nav.left{
    left : -100%; // transformではなくleftで移動
    opacity: 1; // leftの値をセットしてからopacity: 1にする
    will-change: left;
    transition: left ease-in-out;
}

.header__nav.right{
    right : -100%; // transformではなくrightで移動
    opacity: 1; // rightの値をセットしてからopacity: 1にする
    will-change: right;
    transition: right ease-in-out;
}

.header__nav.left.is-open{
    left: 0;
}
.header__nav.right.is-open{
    right: 0;
}

I would appreciate if you could confirm that it works and advise me on the code.
19318dc
a665b30 sorry.. refactor
CodeSandBox
Thank you in advance!

@haru-programming
Copy link
Owner Author

@nakagawamai
度々すみません🙇‍♀️
動作確認の②を変更しています🙇‍♀️
もりけんさんにDiscordでご教示いただいた部分です。

② speedを"" や "abcdefg" など適当な文字に変更する
-> errorがthrowされ、アニメーションがつかない。(開発者にエラーを伝える)

consoleにerrorが表示されます。
合わせてご確認いただけると大変ありがたいです🙇‍♀️
急ぎませんので何卒よろしくお願いたします🙇‍♀️

@nakagawamai
Copy link

nakagawamai commented Mar 24, 2023

@haru-programming
修正ありがとうございます。
こちら、直っているのを確認しました!

I don't know if this is the right solution, but I decided to animate by changing the left or right value instead of tranform.
I also set opacity: 1 after adding the left and right classes so that the menu movement would not be visible.

下記承知しました。

② speedを"" や "abcdefg" など適当な文字に変更する
-> errorがthrowされ、アニメーションがつかない。(開発者にエラーを伝える)

speedに文字列をいれると、ドロワーの動きに speedが影響しなくなるのは、確認できました!
ただ、cosoleには何も表示されないようです。ご確認おねがいいたします🙇‍♀️

consoleにerrorが表示されます。

すみません!console表示もできていました。問題ありません!

@haru-programming haru-programming merged commit cf9aaec into main Mar 25, 2023
@haru-programming haru-programming deleted the feature/lesson31-2 branch March 25, 2023 10:18
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.

3 participants