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

Issue fix #75

Closed
wants to merge 17 commits into from
Closed

Conversation

lvshaoping007
Copy link
Contributor

@lvshaoping007 lvshaoping007 commented Aug 20, 2021

issue fix ,contain
#2, #6, #7, #9, #10, #11, #13, #14, #40, #52, #53

Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

ok on the changes related to #2 #6 #7 #9 #10 #13 #14 #40 #53

would the reporters of those issues (@lukaw3d, @bennetyee) please also leave feedback if you think these aren't sufficient. I reported #40 and it looks like the changes will be sufficient.

I do not see commits related to #44 and #52. could we remove them from the PR description?

requesting changes to separate the version bump into another PR

package.json Outdated
@@ -1,14 +1,12 @@
{
"name": "oasis-wallet",
"version": "0.1.0",
"version": "1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

proposing a version change must be in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i will restore version

@@ -3,7 +3,7 @@
"short_name": "__MSG_appName__",
"description": "__MSG_appDescription__",
"manifest_version": 2,
"version": "0.1.0",
"version": "1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

here as well, separate PR for version bump

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i will restore version too

var index = str.indexOf(startStr);
let lastIndex = -endStr.length
let specialIndex = -1
while (index !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

caveat: the string must have at least one [[...]] pair or the output will be empty

Comment on lines -10 to -11
"storybook": "start-storybook -p 6006 -s public",
"build-storybook": "build-storybook -s public"
Copy link
Member

Choose a reason for hiding this comment

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

Would it be hard to make storybook work? Some story files still exist https://github.com/oasisprotocol/oasis-wallet-ext/tree/master/src/stories

Comment on lines +100 to +108
initLockedState=()=>{
return {
isUnlocked: false,
data: '',
password: '',
currentAccount: {},
mne: ""
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to use this in constructor

-       this.memStore = new ObservableStore({
-           isUnlocked: false,
-           data: '',
-           password: '',
-           currentAccount: {},
-           mne: ""
-       })
+       this.memStore = new ObservableStore(this.initLockedState())

@@ -22,37 +20,26 @@
"base64-arraybuffer": "0.2.0",
"bech32": "2.0.0",
"bignumber.js": "9.0.1",
"bip32": "2.0.6",
Copy link
Member

Choose a reason for hiding this comment

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

yarn.lock file is out of sync

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok , i will fix it

Comment on lines 211 to 221
let cancelText = getLanguage('confirmRestore')
let confirmText = getLanguage('cancelRestore')
let tipImgSrc = txFailed
content = [
getLanguage('restore_tip_1'),
getLanguage('restore_tip_2'),]
ConfirmModal.show({
title,
content,
cancelText,
confirmText,
Copy link
Member

@lukaw3d lukaw3d Aug 21, 2021

Choose a reason for hiding this comment

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

Rename.

  • cancelText and confirmText are reversed.
  • confirmRestore should be confirmReset.
  • onConfirmRestore should be onConfirmReset.
Suggested change
let cancelText = getLanguage('confirmRestore')
let confirmText = getLanguage('cancelRestore')
let tipImgSrc = txFailed
content = [
getLanguage('restore_tip_1'),
getLanguage('restore_tip_2'),]
ConfirmModal.show({
title,
content,
cancelText,
confirmText,
let confirmText = getLanguage('confirmReset')
let cancelText = getLanguage('cancelReset')
let tipImgSrc = txFailed
content = [
getLanguage('restore_tip_1'),
getLanguage('restore_tip_2'),]
ConfirmModal.show({
title,
content,
cancelText,
confirmText,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The button reversed is for users to focus on it. The left side is for confirmation. Generally, users are more accustomed to confirming on the right side, but this operation is very sensitive, so I hope to put it on the left to let users confirm. The reason for the name here is because most of the encapsulated components are canceled on the left, so the name is like this。 but i will change restore to reset , it's better

Comment on lines 238 to 239
<div className={"restore-bottom-container"} onClick={this.onClickRestore}>
<p className="restore-bottom">{getLanguage('resetWallet')}</p>
Copy link
Member

Choose a reason for hiding this comment

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

Move onClick to .restore-bottom element (because .restore-bottom has cursor: pointer)

Comment on lines 35 to 43
renderSpicalStyle = (list,outerIndex) => {
return (
<p className={"wallet-tip-description"} key={outerIndex+""}>
{
list.map((item, index) => {
if (item.type === "common") {
return (<span key={index + ""}>{item.showStr}</span>)
} else {
return (<span key={index + ""} className={"tips-spical"}>{item.showStr}</span>)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is too complex to maintain. Just use Trans element from react-i18next

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The prompts displayed here are passed over. If you use Trans, you may need to pass components over. I don’t find it difficult to maintain, because when you want a special display, you can directly add special symbols. Trans is the same, and The bolding here is not a variable, I don’t think there is any difference between using Trans and the current one

@lvshaoping007
Copy link
Contributor Author

#52 is commit in these commit ,

#44 Here I need to resubmit 44, so i delete #44

@lvshaoping007
Copy link
Contributor Author

i will close this PR and open another one

@pro-wh
Copy link
Contributor

pro-wh commented Aug 23, 2021

confirming that the version change is removed from this PR as requested, thanks

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