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 pickers #55

Merged
merged 3 commits into from
Jan 13, 2016
Merged

Refactor pickers #55

merged 3 commits into from
Jan 13, 2016

Conversation

irisli
Copy link
Contributor

@irisli irisli commented Jan 12, 2016

This PR is a refactor of the Pickers driven by data and the PickerGenerator. This replaces it with a more code-based approach to it.

This also makes pages in the Laboratory persistent between app page change. #44

@@ -17,11 +17,11 @@ export function changePendingRequestProps(props) {
}

export const UPDATE_VALUES = "UPDATE_VALUES";
export function updateValues(param, values) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this action is becoming single-valued, you should reflect it in the name as well: UPDATE_VALUE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I should've been using "values" instead of "value" in the first place.

@nullstyle
Copy link
Contributor

You've copy-pasted the same markup for a "text box with validation" too many times for me to endorse this change.

Instead, I think you should add TextPicker which all of these classes (PubKeyPicker, SecretKeyPicker, PositiveIntPicker, AmountPicker, etc.) use instead of repeating code. Their only differences are the "what validator to use" and "what placeholder to use", as far as I can see. So, for example:

export default function SecretKeyPicker(props) {
  return <TextPicker 
    {...props} 
    placeholder={'Example: SAEXAMPLE6TLGEF6ASOTVTLFUK7LE2K2PFVPFGTEZMMVHH7KLLBBROEQ'}
    validator={validator}
    />
}

@nullstyle
Copy link
Contributor

Other than the copy-pasta, +1

@irisli
Copy link
Contributor Author

irisli commented Jan 13, 2016

Thanks for the useful feedback!

irisli added a commit that referenced this pull request Jan 13, 2016
@irisli irisli merged commit c6fbbe2 into master Jan 13, 2016
@irisli irisli deleted the coded-pickers branch January 13, 2016 22:13
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.

2 participants