-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
1. Added Constants for the OS type in the whole App 2. Added a regex filter for the AIX OS. 3. Added constants for the STORYBOOK_MODE 4. Removed unecessary closing of with or '||' operator #6640
Conversation
…OS in sswitch case statement.
…ng or div and or '||' operator from withSidebarCrossLinks.tsx
…g constant values for the theme modes.
….org into featureBranch
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -1,21 +1,27 @@ | |||
// This is a constant object which is used throughout the app for changing the theme type of the storybook | |||
// This object is read only due to the Object.freez so we can go with it because we are not changing its value anymore | |||
export const STORYBOOK_MODE_THEME = Object.freeze({ |
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.
No need to use Object.freeze, you can use TypeScript as const
@@ -5,9 +5,11 @@ import { useTranslations } from 'next-intl'; | |||
import { useTheme } from 'next-themes'; | |||
import { useEffect, useState } from 'react'; | |||
|
|||
import { STORYBOOK_MODE_THEME } from '@/.storybook/constants'; |
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.
We shouldn't be mixing Storybook constants with Application constants. Make a new set of constants :)
/** | ||
* This are the different types of OS types that are used in the app. | ||
*/ | ||
export const OS = { |
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.
export const OS = { | |
export const USER_OS = { |
switch (osMatch && osMatch[1]) { | ||
case 'Win': | ||
return 'WIN'; | ||
return OS.WIN as UserOS; |
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.
Instead of doing as UserOS
here, you can cast the constant to be a Record<UserOS, UserOS>
or simply do as const
which should be enough,
@@ -13,19 +14,19 @@ export enum OperatingSystem { | |||
export const operatingSystemItems = [ | |||
{ | |||
label: OperatingSystem.WIN, | |||
value: 'WIN' as UserOS, | |||
value: OS.WIN as UserOS, |
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.
No need of as UserOS
if the original constant has a type casting
Description
In this Pull Request I have done following changes:
Validation
I have tested all issues testing it in the UI also I have tested in the debugger for the validation.
npx turbo format
to ensure the code follows the style guide.npx turbo test
to check if all tests are passing.npx turbo build
to check if the website builds without errors.