Skip to content

Conversation

mere-z
Copy link

@mere-z mere-z commented Jul 24, 2025

Description 🧾

Context of PR, summary of changes, as well as any relevant technical decisions/motivations.

Testing

Replace this line with instructions on how to test your changes, as well as any relevant images for UI changes.

Checklist

  • 📍 You have assigned yourself to this pull request.
  • 🔗 You have linked an issue to which this pull request closes.
  • 💭 Leave any relevant specific directions to reviewers.
  • 👀 No secrets in clear text in the pull request.
  • 🎟️ To categorise release notes, the pull request should be labelled with at least one of these labels:
    • feature: for application feature improvement or new features
    • bug: fixing something that previously wasn't working
    • dev: development-side related changes
    • docker: updates to the Docker code
    • setup: changes to the setup/infrastructure of the codebase
    • test: updates to internal testing

Meredith Zhang added 2 commits July 24, 2025 17:25
@mere-z mere-z requested a review from a team as a code owner July 24, 2025 08:30
@mere-z mere-z added 🐞 bug 🐛 Something isn't working 👨🏻‍💻 dev 💻 development-side related issues labels Jul 24, 2025
@mere-z mere-z linked an issue Aug 27, 2025 that may be closed by this pull request
Copy link
Contributor

@mark-trann mark-trann left a comment

Choose a reason for hiding this comment

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

some small nitpicks but lgtm so far!

@@ -23,34 +22,49 @@ Sentry.init({
tracesSampleRate: Number(import.meta.env.VITE_APP_SENTRY_TRACE_RATE_CLIENT),
});

const Root: React.FC = () => {
const hasVisited = localStorage.getItem('visited');
const hasVisited = localStorage.getItem('visited');
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

</ApolloProvider>
);
};
const router = createBrowserRouter(
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation + 1

@@ -402,6 +402,25 @@ const CourseSelect: React.FC<CourseSelectProps> = ({ assignedColors, handleSelec
const isMedium = useMediaQuery(theme.breakpoints.only('md'));
const isTiny = useMediaQuery(theme.breakpoints.only('xs'));

// Each time we select a course, add that course to options
// (removes MUI warning about course selected not being in options)
const mergedOptions = React.useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick:

  const mergedOptions = useMemo(() => {


const decodedColors = colors.map((color) => {
const decodedColor = useColorDecoder(color);
const decodedColor = useColorDecoder(color, currentTheme);
Copy link
Contributor

Choose a reason for hiding this comment

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

the second argument was optional. What was the error that needed this change?

Comment on lines +447 to +448
const { key, ...rest } = props;
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

was the issue here with the key not being assigned correctly?

Comment on lines +108 to +128
<FormControl >
<StyledInputLabel id="select-term-label">Select term</StyledInputLabel>
<CustomStyledSelect
size="small"
labelId="select-term-label"
id="select-term"
label="Select term"
open={open}
onClose={handleClose}
onOpen={handleOpen}
value={termName != '' ? termName.concat(', ', term?.substring(2)) as string : ''}
onChange={selectTerm}
>
{Array.from(termDataStrList).map((term, index) => {
return (
<MenuItem key={index} value={term}>
{term}
</MenuItem>
);
})}
</CustomStyledSelect>
Copy link
Contributor

Choose a reason for hiding this comment

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

fix spacing 😛

@@ -32,13 +34,14 @@ interface ColorThemePreviewProps {
export const ColorThemePreview = ({ previewTheme }: ColorThemePreviewProps) => {
const decodedColors = Object.fromEntries(
Object.entries(colors).map(([key, color]) => {
const decodedColor = useColorDecoder(color, previewTheme);
const { currentTheme } = useContext(AppContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

move this outside the map

@@ -33,15 +33,15 @@ const drawerWidth = 230;
interface StyledDrawerProps {
collapsed: boolean;
isMobile: boolean;
collapsedWidth: number;
collapsedwidth: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

variable name convention is camelCase

@@ -49,9 +49,9 @@ const TimetableTabs: React.FC = () => {

const addTimetabletip = isMacOS ? 'New Tab (Cmd+Enter)' : 'New Tab (Ctrl+Enter)';

const [tabTheme, setTabTheme] = useState<TabTheme>(isDarkMode ? tabThemeDark : tabThemeLight);
const [tabtheme, setTabTheme] = useState<TabTheme>(isDarkMode ? tabThemeDark : tabThemeLight);
Copy link
Contributor

Choose a reason for hiding this comment

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

naming convention

Comment on lines +45 to +47
future: {
v7_relativeSplatPath: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

this helps with future migration to v7 which is good. Curious what prompted you to add this in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug 🐛 Something isn't working 👨🏻‍💻 dev 💻 development-side related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixing react code to be strict mode compliant
2 participants