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 logout #2043

Merged
merged 7 commits into from
Mar 19, 2024
Merged

POST logout #2043

merged 7 commits into from
Mar 19, 2024

Conversation

JoeCohen
Copy link
Member

@JoeCohen JoeCohen commented Mar 19, 2024

  • Uses POST (instead of GET) for logout action.
    This prevents eager loading and may solve the issue of some users being spontaneously logged out.
    It requires rendering Logout as a button (rather than a link).

See #2042

TODO

  • Fix tests to use logout title, or preferably href, instead of funky class.
  • Review stylesheets

Suggest Manual Test

  • On a large screen, hover over the Logout icon for a while to see if it causes eager loading or a spontaneous logout. Then click the link to ensure that it does log you out.
  • Do the equivalent on a small screen.

- Assert presence/absence of logout button, instead of link
- Clicks on button
- This is a 1st draft. It functions, but needs improvement
- Changes logout links to button in the view files
- Includes appropriate changes to css
@JoeCohen JoeCohen linked an issue Mar 19, 2024 that may be closed by this pull request
@coveralls
Copy link
Collaborator

coveralls commented Mar 19, 2024

Coverage Status

coverage: 94.428%. remained the same
when pulling 01eff61 on 2042-post-logout
into 065b407 on main.

@JoeCohen JoeCohen marked this pull request as ready for review March 19, 2024 18:25
@JoeCohen
Copy link
Member Author

@mo-nathan & @nimmolo :
A very short PR to prevent eager load of logout, based on @mo-nathan's email. See #2042.
I think the logout link looks OK in the top navbar.
Can the css be improved?

}
}

.btn-glyphicon-only {
Copy link
Member Author

Choose a reason for hiding this comment

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

@nimmolo

  • Is there a better name for this style?
  • Can its substance be improved?
  • Does it belong in this file or in app/assets/stylesheets/mo/_top_nav.scss?

Copy link
Contributor

Choose a reason for hiding this comment

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

@JoeCohen - I changed some things to make it resemble the current links.

The class will be also useful in Bootstrap 4-5, it's btn-link.
It's for when the element must be a button, but you want it to resemble a link.
I agree that the top nav logout might be better as a button, and i'm looking to rethink all that stuff soon (before upgrading to BS4) but it might make that simpler to keep it as is for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I think .btn-glyphicon-only is not necessary with these changes)

Comment on lines 5 to 9
tag.span(class: "pull-left") do
button_to(:app_logout.t,
account_logout_path,
{ class: "btn", id: "nav_user_logout_link" })
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be centered or otherwise moved?

Copy link
Contributor

Choose a reason for hiding this comment

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

This one currently flushes right, but it doesn't really work with long usernames > "chaelthomas".
I propose knocking it down to the next line... committed that change, let me know if that works.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Trying to avoid the situation where it currently smushes the username on top of the word "logout")

.dropdown-menu li button.btn-link {
padding: 3px 20px;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This just duplicates the Bootstrap inherited styling of .dropdown-menu li a, copied from there

<%= link_to(:welcome_logout_link.t, account_logout_path) %>
<%= button_to(:app_logout.t,
account_logout_path,
{ class: "btn", id: "nav_user_logout_link" }) %>
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc with BS everywhere you use btn you have to also use a coloration, like btn-default.

@JoeCohen JoeCohen merged commit c111b95 into main Mar 19, 2024
5 checks passed
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.

POST logout
3 participants