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

Add Notifications #25

Open
markusphil opened this issue Oct 5, 2019 · 6 comments
Open

Add Notifications #25

markusphil opened this issue Oct 5, 2019 · 6 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@markusphil
Copy link
Collaborator

markusphil commented Oct 5, 2019

Right now error or success messeges are only loged to the console but they need to be displayed for the user.

It might be smart to use vuex for managing the message content. I'd also like to use a transition group for smooth animations.

maybe kinda like this package:
https://www.npmjs.com/package/vue-notification

Thanks to @Twissi we now have a great basic solution for displaying notifications, with more improvements to follow😊 We now should used it for all error handling

@Twissi
Copy link
Contributor

Twissi commented Oct 12, 2019

@markusphil i'd like to work on that

@markusphil
Copy link
Collaborator Author

Yeah awesome! 😊

Twissi added a commit to Twissi/poll_app that referenced this issue Oct 16, 2019
markusphil added a commit that referenced this issue Oct 17, 2019
@dkellner
Copy link
Contributor

After creating a new poll and returning to "Poll Overview", the left column labeled "All Polls" overlap the notification. See attached screenshot:

overlapped-notification

In general it's maybe desirable to close notifications if the user navigates to a different page.

@dkellner
Copy link
Contributor

Another minor issue: When you create a poll with a lot of questions, the "Please enter a poll title" error message appears outside of the visible area.

@markusphil
Copy link
Collaborator Author

@dkellner, Good point! the notifications should have a fixed position.

But I actually like the idea that the noifications stick around even when the user navigates to other pages. Still they should fade out automatically after a few seconds.

@markusphil
Copy link
Collaborator Author

markusphil commented Oct 22, 2019

@Twissi I just merged your new pull request! Thanks a lot! :) When you want to continue working on notifications, you could work on allowing multiple messages and removing them automatically. I guess it should integrate easily with what you've build so far 😊 (You'll need to replace the individual <transition> with a <transition-group> component though). One more comment on your code: As far as I now, you don't need to use the getters object here since you're just using the store's state. As I understand it, Vuex's getters behave just like computed properties in components and you only need them when you need calculations based on the store's state or for combining data from different store modules. Are there other reasons that make working with getters the better choice?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants