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

refactor: migrating to typescript #521

Merged
merged 52 commits into from
Mar 3, 2020
Merged

refactor: migrating to typescript #521

merged 52 commits into from
Mar 3, 2020

Conversation

hanwencheng
Copy link
Contributor

@hanwencheng hanwencheng commented Jan 9, 2020

closes #418.

Include update to React Navigation v4.

This PR I avoid to make changes to all the functionalities and UI features.

But may need to test all the signing issues.

Other changes includes:

  • reorder the file structure
  • use alias for import all components, screens, styles, constants, utils, stores, types
  • update requirement of node to >=10 because of dev dependency fsevents

@hanwencheng hanwencheng mentioned this pull request Jan 21, 2020
14 tasks
jest.config.js Outdated
@@ -14,14 +14,30 @@
// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

const { defaults } = require('jest-config');
// const { defaults } = require('jest-config');
Copy link
Contributor

@Tbaut Tbaut Feb 20, 2020

Choose a reason for hiding this comment

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

nit found while skimming it: stray comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! also deleted other console.log

@hanwencheng hanwencheng mentioned this pull request Feb 21, 2020
Copy link
Contributor

@pmespresso pmespresso left a comment

Choose a reason for hiding this comment

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

ts config looks fine, haven't done a full test yet though.

@sjeohp-zz
Copy link
Contributor

sjeohp-zz commented Mar 2, 2020

Testing on iOS.

  • Adding legacy account doesn't look right.

Screenshot 2020-03-02 at 11 46 49 AM

  • < button is sometimes invisible on all screens. Don't know how to reproduce reliably, it just happens sometimes. Maybe related to the buggy Ethereum deletion below.

IMG_9E6793CB433C-1

  • For any Ethereum account (Substrate / Kusama accounts are fine):
  1. Choose Delete
  2. Alert (Do you really want to delete key pairs for this account) sometimes appears, sometimes does not.
  3. Unlock Identity pin entry fails (You pin must be at least 6 digits long!) even when the pin is correct.
  4. < back button takes me to a blank screen.

IMG_FA72A5713C1B-1

  1. < from the blank screen takes me to the accounts list and the Ethereum account has been deleted.

@sjeohp-zz
Copy link
Contributor

sjeohp-zz commented Mar 2, 2020

  • Account icons sometimes disappear, possibly after the buggy Ethereum deletion.

IMG_6C25F77A1BC2-1

@hanwencheng
Copy link
Contributor Author

Re @sjeohp , thanks for checking it!

  1. Add legacy account screen is only used for developing, it will never shows in production app, so I just use it to check backward-compatibility, I will leave it for now.

  2. Ethereum deletion problem is a vital bug, I have fixed it, also noticed that the alert text with "key pairs" is not clear to the user, so delete all the occurence of "key pairs" and use "account" instead.

  3. With checking back button issue, I tried hard but still can't reproduce it on either Andorid and iOS, probably it is related to ehtereum account deletion problem. I guess it is caused by react navigation, so I updated it to latest version.

  4. When trying reproduce 3, found a new bug, "after screen transition, only after the small delay, buttons on new screen will work." it is fixed by upgrading react navigation.

  5. Network Icon missing problem, it is a bug by misusage of react useEffect hook, with async function in useEffect. The bug is updating UI (in useEffect hook) after the screen is destroyed. So I introduces eslinit-plugin-react-hooks into Eslint, and fixes the bug in both AccoutIcon and QrView.

@hanwencheng hanwencheng merged commit e1785af into master Mar 3, 2020
@hanwencheng hanwencheng deleted the hanwen-typescript branch March 3, 2020 13:16
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.

Migrate to typescript
4 participants