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
This file contains 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
…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 ↗︎
|
mortale2004
changed the title
ovflowd
reviewed
Apr 15, 2024
| @@ -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.
No need to use Object.freeze, you can use TypeScript as const
ovflowd
reviewed
Apr 15, 2024
| @@ -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.
We shouldn't be mixing Storybook constants with Application constants. Make a new set of constants :)
ovflowd
reviewed
Apr 15, 2024
| /** | ||
| * This are the different types of OS types that are used in the app. | ||
| */ | ||
| export const OS = { |
There was a problem hiding this comment.
Suggested change
| export const OS = { | |
| export const USER_OS = { |
ovflowd
reviewed
Apr 15, 2024
| switch (osMatch && osMatch[1]) { | ||
| case 'Win': | ||
| return 'WIN'; | ||
| return OS.WIN as UserOS; |
There was a problem hiding this comment.
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,
ovflowd
reviewed
Apr 15, 2024
| @@ -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.
No need of as UserOS if the original constant has a type casting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
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 formatto ensure the code follows the style guide.npx turbo testto check if all tests are passing.npx turbo buildto check if the website builds without errors.