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

Post session message #129

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dethorpe
Copy link

Completed function for adding post logoff message as a page banner controlled through adding and parsing HTML parameters on refresh.

@dethorpe
Copy link
Author

Hi, small feature add. I hope that I am following the right process for contributing. I am certain of one problem, missing mo/po translations for the new message.

All feedback is appreciated!

@claytondaley
Copy link
Contributor

I like the general idea because we have pages where we don't redirect to ensure that we don't lose on-page data. It'd be great if we could notify a user of the timeout.

My (strictly personal) preference would be to use a modal like the existing timeout warning -- except that it doesn't hide on activity. This would also reduce the number of things that would need to be custom styled.

@dethorpe
Copy link
Author

dethorpe commented Oct 7, 2019

I like the general idea because we have pages where we don't redirect to ensure that we don't lose on-page data. It'd be great if we could notify a user of the timeout.

My (strictly personal) preference would be to use a modal like the existing timeout warning -- except that it doesn't hide on activity. This would also reduce the number of things that would need to be custom styled.

You aren't wrong that a modal would be more in kind with the style of the app and I did consider it but at the end of the day my strictly personal preference wanted it this way. You have my blessing if you want to rewrite the feature however you feels, I just hope that this function in some way will work it's way to the next update.

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.

2 participants