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

Adding BAT token by default to accounts, component patching work #3

Merged
merged 1 commit into from Jun 14, 2019

Conversation

@ryanml
Copy link
Member

ryanml commented Jun 11, 2019

Fixes: brave/brave-browser#4773
Fixes: brave/brave-browser#4924

This PR contains the initial patching strategy for overriding MM components without any actual patches :)

This adds BAT to the user list of tokens by default after creating their account

Ideally this would have been done just by placing the BAT token object in the default store, but this proved to be a little too complicated for a few reasons. A one time add via an existing action proved to be much more simple and reliable:

Main View:
Screen Shot 2019-06-10 at 5 03 21 PM

Non Removable:
Screen Shot 2019-06-10 at 4 57 41 PM

With other assets:
Screen Shot 2019-06-10 at 4 57 32 PM

@ryanml ryanml requested a review from bbondy Jun 11, 2019
@ryanml ryanml self-assigned this Jun 11, 2019
@bbondy bbondy force-pushed the master branch from b478847 to acc9e1e Jun 11, 2019
@ryanml ryanml force-pushed the default-bat branch 3 times, most recently from 93ab9b1 to 9d36048 Jun 11, 2019
@ryanml
Copy link
Member Author

ryanml commented Jun 11, 2019

@bbondy updated to use brave components,

ui/app/pages/index.js Outdated Show resolved Hide resolved
ui/app/pages/routes/index.js Outdated Show resolved Hide resolved
ui/app/store/actions.js Outdated Show resolved Hide resolved
@ryanml ryanml force-pushed the default-bat branch from 9d36048 to 7badd8d Jun 12, 2019
gulpfile.js Outdated Show resolved Hide resolved
@ryanml ryanml force-pushed the default-bat branch from 7badd8d to 0200c4f Jun 13, 2019
])
}

module.exports = connect(mapStateToProps, mapDispatchToProps)(BraveTokenMenuDropdown)

This comment has been minimized.

Copy link
@ryanml

ryanml Jun 13, 2019

Author Member

Unfortunately the way TokenMenuDropdown was set up didn't really make it prime for simple extension, so this is more or less the same file. No copy involved still

This comment has been minimized.

Copy link
@bbondy

bbondy Jun 14, 2019

Member

I guess it's ok for us to fork certain components like this but we should try to avoid when possible.

@ryanml ryanml force-pushed the default-bat branch 2 times, most recently from 1ad2e15 to 0b4b872 Jun 14, 2019
@ryanml ryanml requested a review from bbondy Jun 14, 2019
@ryanml
Copy link
Member Author

ryanml commented Jun 14, 2019

@bbondy this is ready for another look - this approach should be as we discussed, resulting in 0 patches or any copy actions 👍 Some MM components are more straightforward to override than others.

Also, the task do all of the import replacing takes ~1.25 seconds or so, so gulp dev is not slowed down by much.

@ryanml ryanml dismissed bbondy’s stale review Jun 14, 2019

updates made


module.exports = compose(
withRouter,
connect(mapStateToProps, mapDispatchToProps)

This comment has been minimized.

Copy link
@ryanml

ryanml Jun 14, 2019

Author Member

These two (mapStateToProps and mapDispatchToProps) were not exported in the MM component so they are included here.

}
}

BraveHome.propTypes['batTokenAdded'] = PropTypes.bool

This comment has been minimized.

Copy link
@bbondy

bbondy Jun 14, 2019

Member

nit: BraveHome.propTypes.batTokenAdded

gulpfile.js Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
@ryanml ryanml force-pushed the default-bat branch from 0b4b872 to 4ef8d0c Jun 14, 2019
@bbondy
bbondy approved these changes Jun 14, 2019
@ryanml ryanml changed the title Adding BAT token by default to accounts Adding BAT token by default to accounts, component patching work Jun 14, 2019
@ryanml ryanml merged commit e153d33 into master Jun 14, 2019
@ryanml ryanml deleted the default-bat branch Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.