Skip to content

Conversation

@heypoom
Copy link

@heypoom heypoom commented Mar 27, 2018

First of all, I'm sorry for such a large pull request. I originally intended to make a couple of minor refactorings here and there, but I got carried away.

So, this is a fairly major refactoring attempt with quite a lot of alterations. Here are the changes I have made to make the codebase a bit more readable:

  • Refactored the components to make it easier to reason about. This is better seen in the code itself.
  • Added ESLint and Prettier to enforce clear coding style.
  • Implemented styled-components, which might reduce the visual clutter in the code.
  • Reworked the variable naming. The original one is already simple to understand, so I've made a few minor changes.
  • Componentized various parts such as the score display and the hole buttons, and used array mapping to form a grid in order to reduce repetition in the code.
  • Minor tweak to the file structure.
  • Removed unused imports.

Thank you for such a fun little game!

@antronic
Copy link
Contributor

เยอะจัง เดี๋ยวพรุ่งนี้มาตรวจนะ

@heypoom
Copy link
Author

heypoom commented Mar 27, 2018

@antronic ดูแค่ https://github.com/phoomparin/AITH/blob/refactor/src/components/Game.js ก็ได้ครับ

@heypoom
Copy link
Author

heypoom commented Mar 27, 2018

โอ๊ะ ลืมไปว่า Refactor ไปจนไม่ต้อง Pass Props หลายชั้นแล้วแฮะ ไม่ต้องใช้ Context แล้ว~ แก้แปปนะครับ 55

@pariyathida
Copy link
Owner

ขอบคุณนะคะ ^^

@heypoom
Copy link
Author

heypoom commented Mar 28, 2018

แก้ไข Merge Conflict ให้แล้วครับ

} catch (err) {
console.warn('The previous high score cannot be retrieved:', err.message)
}
}
Copy link

@aixasz aixasz Mar 29, 2018

Choose a reason for hiding this comment

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

Where this function used?

If unused please delete it.

Copy link
Author

Choose a reason for hiding this comment

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

This warning message will be triggered when the AsyncStorage throws an error. It is still in use.

Copy link
Author

Choose a reason for hiding this comment

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

Also, componentDidMount is a component lifecycle method which will be invoked by React during runtime.

Copy link

@aixasz aixasz Mar 29, 2018

Choose a reason for hiding this comment

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

Oh, I see.

const highScore = await AsyncStorage.getItem(HIGH_SCORE_KEY)

if (highScore) {
console.log('Retrieved Previous High Score:', highScore)
Copy link

@aixasz aixasz Mar 29, 2018

Choose a reason for hiding this comment

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

This could be Retrieved current high score, right?

Because this state haven't been record the new high score.

Copy link
Author

Choose a reason for hiding this comment

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

Good point! I'll update the log message.

@pankace
Copy link

pankace commented May 16, 2021

just here to say hi, I'm from the future

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.

5 participants