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

Wire up revision 'thanking' #3318

Merged
merged 23 commits into from
Nov 8, 2019
Merged

Wire up revision 'thanking' #3318

merged 23 commits into from
Nov 8, 2019

Conversation

montehurd
Copy link
Contributor

@montehurd montehurd commented Nov 5, 2019

https://phabricator.wikimedia.org/T228790

Based on #3295

Before merging update base to develop (after #3295 is merged)

@montehurd montehurd added Changes welcome Dependent PR PR is dependent on another PR - merge dependent PR first and update branch before merging labels Nov 5, 2019
Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

Looks good! I had some trouble getting a thank to go through on labs, had some token issues but that should all work once we move to prod, for now I faked a success to test the UI. I do think we'll need to remember that this revision was thanked for a selected state when we return to this screen. I'll look into that & fixing the up / down arrow disabled state edge cases tomorrow.

}
guard WMFAuthenticationManager.sharedInstance.isLoggedIn else {
wmf_showLoginOrCreateAccountToThankRevisionAuthorPanel(theme: theme, dismissHandler: nil, loginSuccessCompletion: {
self.apply(theme: self.theme)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why apply the theme here?

Copy link
Contributor Author

@montehurd montehurd Nov 8, 2019

Choose a reason for hiding this comment

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

I do think we'll need to remember that this revision was thanked for a selected state when we return to this screen.

iirc josh said that was a nice-to-have but not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cant remember why the theme is applied there 😂

@tonisevener tonisevener changed the base branch from feature/T228783 to develop November 8, 2019 20:28
@tonisevener tonisevener merged commit 59a4aeb into develop Nov 8, 2019
@tonisevener tonisevener deleted the T228790 branch November 8, 2019 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependent PR PR is dependent on another PR - merge dependent PR first and update branch before merging
Development

Successfully merging this pull request may close these issues.

2 participants