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

change nav item logout link from get link to post button #691

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

k-brahma
Copy link
Contributor

@k-brahma k-brahma commented Jan 6, 2024

As this is the first time tring to contribute OSS, sorry in advance if any of my procedures are not appropriate.

navbar logout link changed to post form.

Description

example/templates/_base.html fixed to:

<li class="nav-item">
<form action="{% url 'logout' %}" method="post" style="display: inline;">
    {% csrf_token %}
    <button type="submit" class="nav-link" style="background: none; border: none; padding-top: 8px; margin: 0; text-decoration: none; cursor: pointer;">
        {% trans "Logout" %}
    </button>
</form>
</li>

Motivation and Context

original link below causes 405 response.
<a class="nav-link" href="{% url 'logout' %}">{% trans "Logout" %}</a>

How Has This Been Tested?

the test below actually ends with some errors but not differ from origin/master's result.

python .\example\manage.py test --settings=tests.settings   

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Contributor

@claudep claudep left a comment

Choose a reason for hiding this comment

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

Thanks for your patch! Indeed, the logout link should be a POST link. However, as the template is bootstrap-style, could you please avoid direct "style=" and replace the styles by matching bootstrap classes (like d-inline, etc.)?

class="nav-link btn btn-link p-2 m-0 text-decoration-none"
@k-brahma
Copy link
Contributor Author

k-brahma commented Jan 7, 2024

Hi, I fixed it.
Think that 'class="nav-link btn btn-link pt-2 m-0 text-decoration-none"' enough.

I visually confirmed that the nav-link works correctly in both expanded and collapsed states.

@@ -41,7 +41,12 @@
<li class="nav-item {% block nav_sessions %}{% endblock %}">
<a class="nav-link" href="{% url 'user_sessions:session_list' %}">{% trans "Sessions" %}</a></li>
<li class="nav-item">
<a class="nav-link" href="{% url 'logout' %}">{% trans "Logout" %}</a>
<form action="{% url 'logout' %}" method="post" style="display: inline;">
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 should still be converted to d-inline class, please.

@k-brahma
Copy link
Contributor Author

k-brahma commented Jan 8, 2024

d-inline

So I launched local sever and checked if the class propery works.
Think it's enough.

Copy link
Contributor

@claudep claudep left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now!

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9159d1c) 95.52% compared to head (98a9e72) 95.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #691   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files          78       78           
  Lines        3354     3354           
  Branches      377      377           
=======================================
  Hits         3204     3204           
  Misses        119      119           
  Partials       31       31           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@claudep claudep merged commit e9dba63 into jazzband:master Jan 8, 2024
8 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.

2 participants