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

feat: Notification Fragment #1622

Merged
merged 1 commit into from
May 14, 2019

Conversation

iamanbansal
Copy link
Contributor

Part of #1617

Changes: fragment setup

Copy link
Member

@nikit19 nikit19 left a comment

Choose a reason for hiding this comment

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

When you submit a WIP PR, it helps if you could point out where you are having problems

@iamanbansal iamanbansal force-pushed the notifications branch 4 times, most recently from 34016ee to ac2bf16 Compare April 17, 2019 10:18
@iamanbansal
Copy link
Contributor Author

@iamareebjamal @nikit19 Need your suggestion on following points

  • where should we show notification button

  • what to implement
    if(userLoggedin) show notification button
    else no button
    or
    onButtonClick navigate user to login if not logged in

  • how to delete notifications? My suggestion is swipe to delete

  • when to mark notification as read?

  • how to open the link from notification message
    Your order has been processed successfully.<br><br><a href='https://eventyay.com/orders/c7883741-3fcd-4801-828d-eaae999f97e7/view/' class='btn btn-info btn-sm'>View Invoice</a>

Current Status
WhatsApp Image 2019-04-17 at 3 59 13 PM

@iamareebjamal
Copy link
Member

Don't delete notifications, just mark read. Show notification button always, in case not logged in we'll show local notifications (planned later)

If there are no local notifications, we'll show Sign Up to see notifications

There'll be a button to mark all notifications as read since there is no notification target as of now

We'll use Html.fromHtml for now. The backend structure for notification will change

notification {
  actor {
    user {
      id: 345
      name: Areeb Jamal
    }
  }
  target {
    order {
      id: 'OH-232353'
      link: 'https://whatevs.com'
    }
  }
  type: order-complete
}

@iamanbansal
Copy link
Contributor Author

todo:
mark notification as read or unread

@iamareebjamal @nikit19 please review

notification 3

@iamanbansal iamanbansal force-pushed the notifications branch 2 times, most recently from 0546929 to 8ec7b5e Compare April 21, 2019 11:28
@iamanbansal iamanbansal changed the title [WIP] Notification Fragment feat: Notification Fragment Apr 21, 2019
@auto-label auto-label bot added the feature label Apr 21, 2019
@iamanbansal
Copy link
Contributor Author

@iamareebjamal @liveHarshit @nikit19 please review

@iamareebjamal
Copy link
Member

PRF

@iamanbansal
Copy link
Contributor Author

PRF

Sorry, what?

@aggarwalpulkit596
Copy link
Contributor

Peer review first 😅

setToolbar(activity, getString(R.string.title_notifications), true)
setHasOptionsMenu(true)

if (notificationViewModel.isLoggedIn()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are you checking again for authentication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To prevent the api call if not logged in.
then in onStart, redirect to login.

Copy link
Member

Choose a reason for hiding this comment

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

But onCreateView is always called after onStart, so can't you remove onStart here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bro no, onStart is called after onActivityCreated
https://developer.android.com/guide/components/fragments

@aggarwalpulkit596
Copy link
Contributor

@aman210697 if you're still working on this please resolve conflicts and if not I'll do it

@iamanbansal
Copy link
Contributor Author

@aman210697 if you're still working on this please resolve conflicts and if not I'll do it

Conflicts are not big, so I'll update it before the approval.
And why do you want to do it again? Just review my PR.

@aggarwalpulkit596
Copy link
Contributor

@aman210697 if you're still working on this please resolve conflicts and if not I'll do it

Conflicts are not big, so I'll update it before the approval.
And why do you want to do it again? Just review my PR.

I was talking about resolving the conflicts

@iamanbansal
Copy link
Contributor Author

iamanbansal commented May 2, 2019

@aman210697 if you're still working on this please resolve conflicts and if not I'll do it

Conflicts are not big, so I'll update it before the approval.
And why do you want to do it again? Just review my PR.

I was talking about resolving the conflicts

Resolved conflicts, and updated to RxKotlin & RxExtensions.. Please review

@anhanh11001
Copy link
Contributor

may I fork this PR and finish it by resolving conflicts?

@iamanbansal
Copy link
Contributor Author

may I fork this PR and finish it by resolving conflicts?

If you haven't started yet, I'll resolve..

@anhanh11001
Copy link
Contributor

@aman210697 oh great

@iamanbansal
Copy link
Contributor Author

resolve conflicts and updated navigation ACTIONS API

@iamareebjamal iamareebjamal merged commit 9ec3104 into fossasia:development May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants