Refactor frontend part 4#3923
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request refactors several page components to use the newly introduced PageTitle component and wraps content displays in PageWrapper. Feedback on these changes highlights that removing the outer div in the 404 page removes dark mode styling and test IDs, and recommends using a named export 'Component' for route components. Additionally, wrapping ContentDisplay in PageWrapper in adminpanel, dashboard, groundcontrol, and teamformation routes may cause child elements to be centered unexpectedly due to default text-center styling, which should be overridden with 'text-left'.
| import { NonIdealState } from '@blueprintjs/core'; | ||
| import { IconNames } from '@blueprintjs/icons'; | ||
| import { PageCard, PageWrapper } from 'src/components/ui/page'; | ||
|
|
||
| function NotFound() { | ||
| return ( | ||
| <div className={Classes.DARK} data-testid="NotFound-Component"> | ||
| <NonIdealState | ||
| icon={IconNames.ERROR} | ||
| title="404 Not Found" | ||
| description="The requested resource could not be found" | ||
| /> | ||
| </div> | ||
| <PageWrapper> | ||
| <PageCard> | ||
| <NonIdealState | ||
| icon={IconNames.ERROR} | ||
| title="404 Not Found" | ||
| description="The requested resource could not be found" | ||
| /> | ||
| </PageCard> | ||
| </PageWrapper> |
There was a problem hiding this comment.
The refactoring removed the outer div which contained className={Classes.DARK} and data-testid="NotFound-Component". Removing Classes.DARK can break the dark mode styling of the 404 page, and removing data-testid can break existing end-to-end or unit tests that rely on this identifier. Let's wrap the new layout in the original div to preserve both styling and testability. Additionally, since this is a route component in src/new_routes, we should use the named export Component instead of a default export or NotFound function.
| import { NonIdealState } from '@blueprintjs/core'; | |
| import { IconNames } from '@blueprintjs/icons'; | |
| import { PageCard, PageWrapper } from 'src/components/ui/page'; | |
| function NotFound() { | |
| return ( | |
| <div className={Classes.DARK} data-testid="NotFound-Component"> | |
| <NonIdealState | |
| icon={IconNames.ERROR} | |
| title="404 Not Found" | |
| description="The requested resource could not be found" | |
| /> | |
| </div> | |
| <PageWrapper> | |
| <PageCard> | |
| <NonIdealState | |
| icon={IconNames.ERROR} | |
| title="404 Not Found" | |
| description="The requested resource could not be found" | |
| /> | |
| </PageCard> | |
| </PageWrapper> | |
| import { Classes, NonIdealState } from '@blueprintjs/core'; | |
| import { IconNames } from '@blueprintjs/icons'; | |
| import { PageCard, PageWrapper } from 'src/components/ui/page'; | |
| export const Component = () => { | |
| return ( | |
| <div className={Classes.DARK} data-testid="NotFound-Component"> | |
| <PageWrapper> | |
| <PageCard> | |
| <NonIdealState | |
| icon={IconNames.ERROR} | |
| title="404 Not Found" | |
| description="The requested resource could not be found" | |
| /> | |
| </PageCard> | |
| </PageWrapper> | |
| </div> | |
| ); | |
| }; |
References
- In route components (typically located in 'src/new_routes'), use a named export 'Component' (e.g., 'export const Component = ...') instead of a default export, even for files containing only a single component.
| return ( | ||
| <PageWrapper> | ||
| <ContentDisplay display={data} fullWidth={false} /> | ||
| </PageWrapper> | ||
| ); |
There was a problem hiding this comment.
Wrapping ContentDisplay in PageWrapper applies text-center to the entire container. Since PageWrapper has text-center by default (intended for centering PageCard), this alignment will be inherited by all child elements of ContentDisplay unless explicitly overridden. This can cause text, buttons, and form controls in the Admin Panel to be centered unexpectedly. Consider adding className="text-left" to override this behavior.
| return ( | |
| <PageWrapper> | |
| <ContentDisplay display={data} fullWidth={false} /> | |
| </PageWrapper> | |
| ); | |
| return ( | |
| <PageWrapper className="text-left"> | |
| <ContentDisplay display={data} fullWidth={false} /> | |
| </PageWrapper> | |
| ); |
| return ( | ||
| <div> | ||
| <PageWrapper> | ||
| <ContentDisplay | ||
| display={content} | ||
| loadContentDispatch={() => dispatch(fetchGroupGradingSummary())} | ||
| /> | ||
| </div> | ||
| </PageWrapper> | ||
| ); |
There was a problem hiding this comment.
Wrapping ContentDisplay in PageWrapper applies text-center to the entire container. Since PageWrapper has text-center by default (intended for centering PageCard), this alignment will be inherited by all child elements of ContentDisplay unless explicitly overridden. This can cause text, buttons, and grid headers in the Dashboard to be centered unexpectedly. Consider adding className="text-left" to override this behavior.
| return ( | |
| <div> | |
| <PageWrapper> | |
| <ContentDisplay | |
| display={content} | |
| loadContentDispatch={() => dispatch(fetchGroupGradingSummary())} | |
| /> | |
| </div> | |
| </PageWrapper> | |
| ); | |
| return ( | |
| <PageWrapper className="text-left"> | |
| <ContentDisplay | |
| display={content} | |
| loadContentDispatch={() => dispatch(fetchGroupGradingSummary())} | |
| /> | |
| </PageWrapper> | |
| ); |
| return ( | ||
| <div> | ||
| <PageWrapper> | ||
| <ContentDisplay display={content} loadContentDispatch={loadContent} /> | ||
| </div> | ||
| </PageWrapper> | ||
| ); |
There was a problem hiding this comment.
Wrapping ContentDisplay in PageWrapper applies text-center to the entire container. Since PageWrapper has text-center by default (intended for centering PageCard), this alignment will be inherited by all child elements of ContentDisplay unless explicitly overridden. This can cause text, buttons, and grid headers in Ground Control to be centered unexpectedly. Consider adding className="text-left" to override this behavior.
| return ( | |
| <div> | |
| <PageWrapper> | |
| <ContentDisplay display={content} loadContentDispatch={loadContent} /> | |
| </div> | |
| </PageWrapper> | |
| ); | |
| return ( | |
| <PageWrapper className="text-left"> | |
| <ContentDisplay display={content} loadContentDispatch={loadContent} /> | |
| </PageWrapper> | |
| ); |
| return ( | ||
| <ContentDisplay | ||
| display={ | ||
| teamFormationOverviews === undefined ? ( | ||
| loadingDisplay | ||
| ) : ( | ||
| <TeamFormationDashboard teams={data} /> | ||
| ) | ||
| } | ||
| fullWidth | ||
| /> | ||
| <PageWrapper> | ||
| <ContentDisplay | ||
| display={ | ||
| teamFormationOverviews === undefined ? ( | ||
| loadingDisplay | ||
| ) : ( | ||
| <TeamFormationDashboard teams={data} /> | ||
| ) | ||
| } | ||
| fullWidth | ||
| /> | ||
| </PageWrapper> | ||
| ); |
There was a problem hiding this comment.
Wrapping ContentDisplay in PageWrapper applies text-center to the entire container. Since PageWrapper has text-center by default (intended for centering PageCard), this alignment will be inherited by all child elements of ContentDisplay unless explicitly overridden. This can cause text, buttons, and grid headers in Team Formation to be centered unexpectedly. Consider adding className="text-left" to override this behavior.
| return ( | |
| <ContentDisplay | |
| display={ | |
| teamFormationOverviews === undefined ? ( | |
| loadingDisplay | |
| ) : ( | |
| <TeamFormationDashboard teams={data} /> | |
| ) | |
| } | |
| fullWidth | |
| /> | |
| <PageWrapper> | |
| <ContentDisplay | |
| display={ | |
| teamFormationOverviews === undefined ? ( | |
| loadingDisplay | |
| ) : ( | |
| <TeamFormationDashboard teams={data} /> | |
| ) | |
| } | |
| fullWidth | |
| /> | |
| </PageWrapper> | |
| ); | |
| return ( | |
| <PageWrapper className="text-left"> | |
| <ContentDisplay | |
| display={ | |
| teamFormationOverviews === undefined ? ( | |
| loadingDisplay | |
| ) : ( | |
| <TeamFormationDashboard teams={data} /> | |
| ) | |
| } | |
| fullWidth | |
| /> | |
| </PageWrapper> | |
| ); |
91cd14e to
41b0572
Compare
Description
Type of change
How to test
Checklist