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

[6951] apps/budgeting: end session on button click #4878

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

fuzzylogic2000
Copy link
Contributor

Oh, this feels very unrefined and clunky. For the logout, we do a post, so maybe we should also do a post instead of the get here after all? And maybe some different permissions? (@Rineee @goapunk)
And in this version the button works, but you can only see that after reloading the page. I think, there was more to fix also when the session ends, so that the form field to fill in returns? Buuut we currently need a reload of the whole page for that anyway. 😢 The React is also of course missing the modal inbetween the first click and ending the session and doesn't look like it should (link). (@khamui @phillimorland )

@khamui
Copy link
Contributor

khamui commented Jan 30, 2023

link comes with modal in this commit:
847fd76

@@ -16,6 +16,7 @@ export const BudgetingProposalList = (props) => {
return django.interpolate(countText, [votes])
}
const noResults = django.gettext('Nothing to show')
const endSessionString = django.gettext('Are you ready? End session')
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be are you finished right? are you ready doesn't really make sense in the context

@philli-m philli-m self-assigned this Feb 1, 2023
@philli-m
Copy link
Contributor

philli-m commented Feb 1, 2023

There is no way to load the django token form other then a page reload which I have added.
The session ending does not work in firefox (I think due to caching but will have a look), works in Chrome and Safari.

@goapunk or @Rineee i'm not sure on the get or post stuff if either of you have an opinion i can implement while trying to fix the firefox caching?

This will be looked at in filter issue Also due to the url removing the parameters when after user added a token, on ending the session the page returns to map view as that is the default without any parameters, attempted to add them back with replace() however this doesn't work but as the page also reverts to the map view on clicking refresh btn, which is what the reload() function imitates i'm not sure we care?!

@goapunk
Copy link
Contributor

goapunk commented Feb 2, 2023

I'm fine with get, the session ending works for me in firefox, maybe there's a difference between mac and linux?

@philli-m

This comment was marked as outdated.

@goapunk

This comment was marked as outdated.

@philli-m

This comment was marked as outdated.

@khamui

This comment was marked as outdated.

@philli-m

This comment was marked as outdated.

@philli-m philli-m force-pushed the kl-2023-01-end-session branch from 93bbdd9 to 5bd8987 Compare February 2, 2023 10:37
@philli-m philli-m changed the title WIP: [6951] apps/budgeting: end session on button click [6951] apps/budgeting: end session on button click Feb 2, 2023
@philli-m philli-m requested review from Rineee, khamui and goapunk February 2, 2023 10:43
@philli-m philli-m removed their assignment Feb 2, 2023
@philli-m philli-m force-pushed the kl-2023-01-end-session branch from ae80fc3 to cbce40f Compare February 2, 2023 11:06
Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

looks good to me, the wording can be changed with the modal? or should we wait till the modal PR is merged?

@philli-m
Copy link
Contributor

philli-m commented Feb 2, 2023

@goapunk no I think this should be merged then @khamui can rebase and add modal stuff

@goapunk goapunk merged commit 340c916 into main Feb 2, 2023
@goapunk goapunk deleted the kl-2023-01-end-session branch February 2, 2023 12:12
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.

4 participants