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

261 logout pop up #427

Merged

Conversation

pniedzwiedzinski
Copy link
Member

I added modal that will show the warning to user attempting logout on offline.

I also moved routes to a separate file because there were problem with import, that finally wasn't needed. But I think it's ok to leave it that way.

Copy link
Member

@arturtamborski arturtamborski left a comment

Choose a reason for hiding this comment

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

Minor comments from my side, mostly regarding translation.

import { reverseObject } from '../services/reverseObject';

const { user } = store.state;
const { user } = store ? store.state : { user: undefined };
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { user } = store ? store.state : { user: undefined };
const { user } = store && store.state ? store.state : { user: undefined };

?

frontend/src/mixins/GroupGuardMixin.js Outdated Show resolved Hide resolved
frontend/src/router/index.js Show resolved Hide resolved
frontend/src/store/actions.js Outdated Show resolved Hide resolved
frontend/src/translations.json Outdated Show resolved Hide resolved
frontend/src/translations.json Outdated Show resolved Hide resolved
frontend/src/translations.json Outdated Show resolved Hide resolved
@arturtamborski arturtamborski linked an issue Feb 12, 2020 that may be closed by this pull request
@arturtamborski
Copy link
Member

I have a question - should this modal be displayed only when user is offline? It's not described as that in the linked issue (although there isn't much detail either), so maybe we should discuss this?

Other thing - I'm not an UX guy, don't know much about that but I thought it would be nice to have the buttons separated from each other, just as we did in DriveFromView with Submit and Reset.
What do you think about it?

@monikakrawczak
Copy link
Contributor

Confirmed with Client, that we need displaying pop up when user is online as well.
I'm opening a new task.

@arturtamborski
Copy link
Member

No need to, I think that all work can be done in this PR, as it's still open :)

@arturtamborski
Copy link
Member

In case this was lost somehow: we'd love to display the pop-up developed here no matter the network status (online/offline). Currently it works so it's shown only when user is offline - this can be surprising when the user temporarily gains network connection.

Please modify the PR such that the pop-up always pops up - thanks!

@pniedzwiedzinski
Copy link
Member Author

Sorry for the delay but I was unable to get on this. I think in the next meeting I will end this as it sounds easy to implement.

@pniedzwiedzinski
Copy link
Member Author

This should be ok now

Copy link
Member

@arturtamborski arturtamborski left a comment

Choose a reason for hiding this comment

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

Sorry for not reviewing it earlier. Everything looks fine, I've tested it and it works just the way we wanted it, thanks for this Pull Request :)

@arturtamborski arturtamborski merged commit 0461967 into CodeForPoznan:develop Mar 5, 2020
@pniedzwiedzinski pniedzwiedzinski deleted the 261-logout-pop-up branch March 5, 2020 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log-out confirmation pop-up
3 participants