search

0xProject / 0x-launch-kit-frontend

Apache License 2.0
114 stars 208 forks source link

(Feature) - Limit number of notifications #172

Closed mariano-aguero closed 5 years ago

mariano-aguero commented 5 years ago

Closes #161

unjapones commented 5 years ago

@fvictorio

On one hand, I like the idea of limiting the number of notifications we save in local storage, but not the ones shown in the dropdown in a session. On the other hand, maybe I would expect that the notifications limit applies to the existing notifications, not only the ones shown after a refresh.

Well, if it is for simplicity I would go with your 2nd sentence "notifications limit applies to the existing notifications, not only the ones shown after a refresh"; i.e. keep the notifications limited to a fixed number and discard old notifications as new ones appear (if I got it right). And yes... it is simple but not the ideal approach.

p/s: I also like limiting what we store in local storage.

unjapones commented 5 years ago

@fvictorio @mariano-aguero @pablofullana just in case, my position here is to keep it simple and stick to what the PR currently does: limit the items saved in localstorage and just display these amount of items in the notifications section.

If we want to improve this to show the current session's new notifications over the save limit, I would probably check with the team/0x if they want the feature and create an issue.

fvictorio commented 5 years ago

If we want to improve this to show the current session's new notifications over the save limit, I would probably check with the team/0x if they want the feature and create an issue.

To clarify: this is what the PR does. The limitation is only for the amount of notifications saved. I think it's OK either way.