-
Notifications
You must be signed in to change notification settings - Fork 4
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 tdbytes info page and suggestions #283
Conversation
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.
Nice work! See comment about changing some text on the td-bytes page
lΓ¦ttis pΓ₯. | ||
</Text> | ||
<Text> | ||
Kontoret skal i utgangspunktet vΓ¦re Γ₯pent i skoletidene, men |
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.
Maybe change info text now that all IFI-students should have access to td-kontoret
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.
Generally it looks good π I left one comment on the nature of the suggestion route and maybe it could be a private route allowing the private route to handle any missing authentication
if (role === Roles.admin || role === Roles.kiosk_admin) { | ||
fetchSuggestions(); | ||
} else { | ||
setErrMsg('Du har ikke de nΓΈdvendige rettighetene'); |
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.
Just a question. is it an error if I am on this page as a regular user? Shouldn't the fetch only happen if I have the right access and nothing else if I am a regular user?
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.
As this is the only kiosk_admin site so far, I think it's reasonable to do the auth check in the page instead of creating another route component to handle it
src/App.tsx
Outdated
@@ -65,6 +67,8 @@ const App: React.FC = () => { | |||
/> | |||
<Route path="/about-us" component={AboutPage} /> | |||
<Route path="/new-student" component={NewStudentsPage} /> | |||
<Route path="/tdbytes/suggestions" component={SuggestionsPage} /> |
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.
Is this really an open route? isn't this only for members i.e PrivateRoute
?
const moveToLoginPage = () => { | ||
history.push('/login'); | ||
}; |
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.
if the suggestion route is moved to a privet route this can be removed as a the privateRoute sends an unauthorised user to login before redirecting the back to the desired destination. So redirecting to /suggestion
automatically sends the user to login if not authenticated
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'm thinking the TDBytes page should be viewable to anyone, also if they aren't logged in. Posting suggestions however should require login.
π Changes
Closes #274
β¨ Added a tdBytes page
Displays some general info about the kiosk and office
Has a small form at the bottom to submit kiosk product suggestions
β¨ Added a page to view kiosk product suggestions
Displays all suggestions in a table
π Also added a new kiosk admin role, to allow members outside TD-Styret to view suggestions
Additional changes