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

Feat: implement the top-right usertools block #2324

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Aug 27, 2024

Closes #2246

This PR is the last part of #2246 and it implements the top right usertools block for in-person eligibility/enrollment pages. It overrides the text-transform property it inherits from Django's base admin site so that the welcome message is capitalized (WELCOME, USER) but the sign out message isn't (Sign out). It uses the same idea to remove the underline from Sign out. In addition, it adds a Bootstrap icon and uses it as the Sign out icon.

@lalver1 lalver1 self-assigned this Aug 27, 2024
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Aug 27, 2024
Copy link

Coverage report

This PR does not seem to contain any modification to coverable code.

@lalver1 lalver1 marked this pull request as ready for review August 27, 2024 17:04
@lalver1 lalver1 requested a review from a team as a code owner August 27, 2024 17:04
@thekaveman thekaveman changed the title Feat: implement the top-right {% usertools %} block Feat: implement the top-right usertools block Aug 27, 2024
@thekaveman
Copy link
Member

I edited the title, please don't put code like that in PR titles.

Comment on lines 61 to 67

#user-tools,
#logout-form button {
padding: 0;
font-weight: 300;
font-size: 0.6875rem;
letter-spacing: 0.5px;
Copy link
Member

Choose a reason for hiding this comment

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

I believe this CSS Is unneccessary. These styles are already being set by base.css. The only one necessary is text-transform: unset.

Copy link
Member

Choose a reason for hiding this comment

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

If we're okay with Sign Out instead of Sign out, we can use Bootstrap's text-capitalize class instead. Unfortunately there is no text-unset class for text-transform: unset...... Then we could get all the styles in via utility classes (the preferred method).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, thanks for pointing that out. I only left the necessary CSS so that we match the capitalization in the mock-up (Sign out).

<img class="icon" width="15" height="15" src="{% static "img/icon/box-arrow-right.svg" %}" alt="" />
<form id="logout-form" method="post" action="{% url 'admin:logout' %}">
{% csrf_token %}
<button type="submit">{% translate "Sign out" %}</button>
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that makes sense. Cleaned it up in aa73dca and 51f2462

{% block welcome-msg %}
<span class="text-uppercase text-white">
{% translate "Welcome," %}
<strong>{% firstof user.get_short_name user.get_username %}</strong>. </span>
Copy link
Member

@machikoyasuda machikoyasuda Aug 27, 2024

Choose a reason for hiding this comment

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

<span class="fw-bold"> instead of <strong> - the <strong> doesn't make the font bold for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, made the change in aa73dca

@machikoyasuda
Copy link
Member

Just some small changes @lalver1 - otherwise looks good to me. Ping me if you have any Qs!

Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

Thanks @lalver1! Let's make sure @machikoyasuda approves too

Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

LGTM 💯 💯 💯

@lalver1 lalver1 merged commit fe34975 into main Aug 27, 2024
10 checks passed
@lalver1 lalver1 deleted the feat/in-person-usertools branch August 27, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transit agency staff should see a home dashboard upon logging in
4 participants