search

codeforboston / cliff-effects

Cliff effects guidance prototype (archived)
https://codeforboston.github.io/cliff-effects/#/
MIT License
30 stars 64 forks source link

Style cleans '/src/components' through 'ExternalLink.js'. #1018 #1023

Closed knod closed 5 years ago

dylanesque commented 5 years ago

I don't understand why the const declarations here were changed to let.

knod commented 5 years ago

As far as const vs. let, I had thought the pre-existing choice (which hadn't been implemented) was to use let by default. I can't seem to find my notes on it now and I might be wrong about that.

I know it's something that needs more discussion. Is it a blocking change for this PR?

dylanesque commented 5 years ago

As far as const vs. let, I had thought the pre-existing choice (which hadn't been implemented) was to use let by default. I can't seem to find my notes on it now and I might be wrong about that.

I know it's something that needs more discussion. Is it a blocking change for this PR?

For me, this is blocking, it makes the code that was altered more fragile. Values like destructured props should not change, and really should always be declared with const.

knod commented 5 years ago

@dylanesque : I hear you feel strongly about that. I'll change back the destructuring ones. If you want a change, though, it's often a good idea to comment on the relevant lines with a 'request changes' review.

knod commented 5 years ago

Dang, also forgot to destructure some props in there!

knod commented 5 years ago

Values like destructured props should not change

Did you mean destructured this.state? I don't think I changed any destructured props.

dylanesque commented 5 years ago

Values like destructured props should not change

Did you mean destructured this.state? I don't think I changed any destructured props.

I meant "like" props, so state is similar enough in this example.