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

Add link to admin page #553

Merged
merged 12 commits into from
Apr 16, 2024
Merged

Add link to admin page #553

merged 12 commits into from
Apr 16, 2024

Conversation

podliashanyk
Copy link
Contributor

Closes #537

@podliashanyk podliashanyk added APIv2 Compatibility with APIv2 is guaranteed, may or may not be compatible with other API versions user administration labels Apr 10, 2024
@podliashanyk podliashanyk self-assigned this Apr 10, 2024
@podliashanyk podliashanyk force-pushed the add-link-to-admin-page branch from 2f0c73e to e0c56fa Compare April 11, 2024 07:57
@podliashanyk podliashanyk marked this pull request as ready for review April 11, 2024 14:27
@podliashanyk podliashanyk force-pushed the add-link-to-admin-page branch from 5c80469 to 571e87b Compare April 11, 2024 14:34
Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

I think it looks great, I really like the new design with the icons and the horizontal divider.

My only problem is the fact that the admin_url is an absolute url and I think you're treating it as a relative url, which results in if I get admin_url: http://localhost:8000/admin/ then the link I'm clicking on is http://localhost:8080/http://localhost:8000/admin/, which interestingly enough first gave me a 404 error, but now actually leads me to the admin.
But it still should only link to the admin_url exactly how it's received.

@podliashanyk podliashanyk force-pushed the add-link-to-admin-page branch from 571e87b to 325fa83 Compare April 15, 2024 13:20
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 57.29%. Comparing base (09e138b) to head (325fa83).

Files Patch % Lines
src/components/header/Header.tsx 33.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
- Coverage   57.30%   57.29%   -0.02%     
==========================================
  Files          86       86              
  Lines        3687     3693       +6     
  Branches      835      817      -18     
==========================================
+ Hits         2113     2116       +3     
- Misses       1565     1568       +3     
  Partials        9        9              

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

Since admin_url it is always an absolute URL
For better accessibility
Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

Perfect! Tested both with an admin account and a normal account and all works as expected!

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

Nice!

@podliashanyk podliashanyk merged commit 803c7e9 into master Apr 16, 2024
3 checks passed
@podliashanyk podliashanyk deleted the add-link-to-admin-page branch April 16, 2024 13:15
@podliashanyk podliashanyk mentioned this pull request Apr 19, 2024
podliashanyk added a commit that referenced this pull request Apr 22, 2024
podliashanyk added a commit that referenced this pull request Apr 22, 2024
* Update changelog for PR #552

* Update changelog for PR #553

* Update changelog for PR #557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIv2 Compatibility with APIv2 is guaranteed, may or may not be compatible with other API versions user administration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a link to the admin page
4 participants