{{ message }}
feat(ui): enhance not-found page #1379
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheNando
commented
Apr 18, 2025
There was a problem hiding this comment.
I mimicked the layout from the DisabledUser page, for consistency.
|
looks wise - much better! |
specter-flq
reviewed
Apr 22, 2025
specter-flq
reviewed
Apr 22, 2025
Comment on lines
+31
to
+36
| useEffect(() => { | ||
| if (isFullyAuthenticated) { | ||
| return; | ||
| } | ||
|
|
||
| navigate(ROUTE_LOGIN); |
There was a problem hiding this comment.
This works and make sense but was curious as to that vs
useEffect(() => {
if (!isFullyAuthenticated) {
navigate(ROUTE_LOGIN);
}
}, [isFullyAuthenticated, navigate]);
is there a benefit or consideration in having the positive conditional return ?
There was a problem hiding this comment.
This referred to as the "exit early" pattern. There are typically many benefits to which objectively improve code quality. You can read about it here. However, in this case, there is not much reason to pick this over your option than habit and consistency.
There was a problem hiding this comment.
Nice! Thanks for that article, read and bookmarked :)
specter-flq
reviewed
Apr 22, 2025
cmd/ui/src/views/NotFound.tsx
Outdated
Comment on lines
41
to
63
| <Grid container spacing={4} justifyContent='center'> | ||
| <Grid item xs={12}> | ||
| <Alert severity='warning'> | ||
| <AlertTitle>404 - Page not found</AlertTitle> | ||
| There is no page associated with this route. Please contact your system administrator for | ||
| assistance. | ||
| </Alert> | ||
| </Grid> | ||
| <Grid item xs={12}> | ||
| <Box mt={2}> | ||
| <Button | ||
| => { | ||
| navigate(ROUTE_EXPLORE); | ||
| }} | ||
| data-testid='page-not-found-go-to-explore' | ||
| size='large' | ||
| type='button' | ||
| className='w-full'> | ||
| Go to Explore | ||
| </Button> | ||
| </Box> | ||
| </Grid> | ||
| </Grid> |
There was a problem hiding this comment.
( What we spoke about yesterday but figured I would add here just in case ) Not a showstopper, but could we maybe consider changing Box and Grid components to just the html tags with TW? We are trying to get some MUI components out of the codebase as we go.
|
Works and looks great! Good idea on mimicking the existing one! just a couple of comments and a curiosity question. |
5eb092b to
7aebe6e
Compare
specter-flq
approved these changes
Apr 22, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Update NotFound page to clearly state 404 status and to redirect to login if unauthenticated.
Motivation and Context
Resolves BED-5326
How Has This Been Tested?
Manually tested. Visited bad routes while authenticated and again while unauthenticated.
ex. http://bloodhound.localhost/ui/not-a-route
Screenshots:
Before
After
Types of changes
Checklist: