-
Notifications
You must be signed in to change notification settings - Fork 137
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
Creating unauthorized page #484
Creating unauthorized page #484
Conversation
<div className={styles.errtext}> | ||
<h1>Oops!</h1> | ||
<h3>Looks like you don&apost have permission to access this page.</h3> | ||
<div className={styles.buttondiv}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be don't
(you missed a semicolon)
} | ||
.buttondiv { | ||
display: flex !important; | ||
justify-content: center; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why this !important
selector was added. Maybe we could remove it, I don't see any problems without it. (@daltonfury42)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's remove. 👍
flex-wrap: wrap; | ||
div { | ||
padding: 1rem; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's reduce the padding to 0.5 rem
@@ -14,6 +14,8 @@ const handleApiErrors = (err) => { | |||
`There's a problem with the data you've entered ${err.response.data.message}` | |||
) | |||
); | |||
} else if (err.response.status === 401) { | |||
window.location.href = `${window.location.origin}/unauthorized`; | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daltonfury42 what do you think about this approach? Should we go ahead with this or conditionally switch to the unauthorized page from within the admin component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't know. I am trying to think of what could go wrong in our current implementation if we go this route. 🤔
If we want to conditionally switch to unauthorised page from admin component, we need to let admin component know that it was 401. But we took the call to handle and swallow API errors at one place in here, so it might be difficult...(It's a design limitation). Do you see a way?
If not, can we go ahead with this, and then see if it doesn't cause problems? (I can't see any right now, though it might give issues later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay let's do this for now and change later if needed.
Another thing is, this page is almost identical to the 404 page we have. Maybe we could make a standard error page component and pass in props for text, buttons and image for both the 404 page and this unauthorized page. |
If we are planing to add CreateJoinForm to the 404 page, it might come in the way, so we could keep them separate for now, and combine them later? @maaverik |
Yes, forgot about that. Let's keep them separate. |
Could you merge with the current master to take care of these merge issues? Also, any thoughts on adding an image? |
@mathkruger are you looking to complete the PR? |
Closing this. @mathkruger you can reopen if you want to resume sometime. |
Fix for issue #476