Skip to content
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 a regex filter for the AIX OS. 2. Added constants for the STORYBOOK_MODE 3.Removed unecessary closing of with or '||' operator #6635

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions .storybook/constants.ts
Original file line number Diff line number Diff line change
@@ -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({
dark: 'dark',
light: 'light',
});
// This defines "execution" modes that Chromatic will run on the each Storybook Story
// This allows us to test each Story with different parameters
// @see https://www.chromatic.com/blog/introducing-story-modes/
export const STORYBOOK_MODES = {
'dark mobile': {
theme: 'dark',
theme: STORYBOOK_MODE_THEME.dark,
viewport: 'small',
},
'dark desktop': {
theme: 'dark',
theme: STORYBOOK_MODE_THEME.dark,
viewport: 'large',
},
'light mobile': {
theme: 'light',
theme: STORYBOOK_MODE_THEME.light,
viewport: 'small',
},
'light desktop': {
theme: 'light',
theme: STORYBOOK_MODE_THEME.light,
viewport: 'large',
},
};
Expand Down
13 changes: 10 additions & 3 deletions .storybook/preview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import { NextIntlClientProvider } from 'next-intl';

import { withThemeByDataAttribute } from '@storybook/addon-themes';
import { NotificationProvider } from '@/providers/notificationProvider';
import { STORYBOOK_MODES, STORYBOOK_SIZES } from '@/.storybook/constants';
import {
STORYBOOK_MODES,
STORYBOOK_SIZES,
STORYBOOK_MODE_THEME,
} from '@/.storybook/constants';
import type { Preview, ReactRenderer } from '@storybook/react';

import englishLocale from '@/i18n/locales/en.json';
Expand All @@ -29,8 +33,11 @@ const preview: Preview = {
</NextIntlClientProvider>
),
withThemeByDataAttribute<ReactRenderer>({
themes: { light: '', dark: 'dark' },
defaultTheme: 'light',
themes: {
light: STORYBOOK_MODE_THEME.light,
dark: STORYBOOK_MODE_THEME.dark,
},
defaultTheme: STORYBOOK_MODE_THEME.light,
attributeName: 'data-theme',
}),
],
Expand Down
2 changes: 1 addition & 1 deletion app/[locale]/[[...path]]/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const generateMetadata = async ({ params }: DynamicParams) => {

// Gets all mapped routes to the Next.js Routing Engine by Locale
const mapRoutesForLocale = async (locale: string) => {
const routesForLanguage = await dynamicRouter.getRoutesByLanguage(locale);
const routesForLanguage = dynamicRouter.getRoutesByLanguage(locale);

return routesForLanguage.map(pathname =>
dynamicRouter.mapPathToRoute(locale, pathname)
Expand Down
4 changes: 3 additions & 1 deletion components/Common/Search/States/WithPoweredBy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

import styles from './index.module.css';

const getLogoURL = (theme: string = 'dark') =>
const getLogoURL = (theme: string = STORYBOOK_MODE_THEME.dark) =>
`https://website-assets.oramasearch.com/orama-when-${theme}.svg`;

export const WithPoweredBy = () => {
Expand Down
17 changes: 11 additions & 6 deletions components/Common/ThemeToggle/__tests__/index.test.mjs
Original file line number Diff line number Diff line change
@@ -1,32 +1,37 @@
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import { STORYBOOK_MODE_THEME } from '@/.storybook/constants';

import ThemeToggle from '../';

let mockCurrentTheme = 'light';
let mockCurrentTheme = STORYBOOK_MODE_THEME.light;

const toggleTheme = () => {
mockCurrentTheme = mockCurrentTheme === 'light' ? 'dark' : 'light';
mockCurrentTheme =
mockCurrentTheme === STORYBOOK_MODE_THEME.light
? STORYBOOK_MODE_THEME.dark
: STORYBOOK_MODE_THEME.light;
};

describe('ThemeToggle', () => {
let toggle;

beforeEach(() => {
mockCurrentTheme = 'light';
mockCurrentTheme = STORYBOOK_MODE_THEME.light;

render(<ThemeToggle onClick={toggleTheme} />);
toggle = screen.getByRole('button');
});

it('switches dark theme to light theme', async () => {
mockCurrentTheme = 'dark';
mockCurrentTheme = STORYBOOK_MODE_THEME.dark;
await userEvent.click(toggle);
expect(mockCurrentTheme).toBe('light');
expect(mockCurrentTheme).toBe(STORYBOOK_MODE_THEME.light);
});

it('switches light theme to dark theme', async () => {
await userEvent.click(toggle);
expect(mockCurrentTheme).toBe('dark');
expect(mockCurrentTheme).toBe(STORYBOOK_MODE_THEME.dark);
});
});
4 changes: 2 additions & 2 deletions components/withBlogCrossLinks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ const WithBlogCrossLinks: FC = async () => {

return (
<div className="mt-4 grid w-full grid-cols-2 gap-4 xs:grid-cols-1">
{(previousCrossLink && (
{previousCrossLink && (
<CrossLink
type="previous"
text={previousCrossLink.title}
link={previousCrossLink.slug}
/>
)) || <div />}
)}

{nextCrossLink && (
<CrossLink
Expand Down
7 changes: 6 additions & 1 deletion components/withNavBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { useLocale } from 'next-intl';
import { useTheme } from 'next-themes';
import type { FC } from 'react';

import { STORYBOOK_MODE_THEME } from '@/.storybook/constants';
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned before, we shouldn't have Sotrybook constants being imported in Application code, you can create a new constant similar to the Storybook one, on an App constants file, like next.constants.mjs

import NavBar from '@/components/Containers/NavBar';
import WithBanner from '@/components/withBanner';
import { useClientContext, useSiteNavigation } from '@/hooks';
Expand All @@ -19,7 +20,11 @@ const WithNavBar: FC = () => {
const locale = useLocale();

const toggleCurrentTheme = () =>
setTheme(resolvedTheme === 'dark' ? 'light' : 'dark');
setTheme(
resolvedTheme === STORYBOOK_MODE_THEME.dark
? STORYBOOK_MODE_THEME.light
: STORYBOOK_MODE_THEME.dark
);

return (
<div>
Expand Down
4 changes: 2 additions & 2 deletions components/withSidebarCrossLinks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ const WithSidebarCrossLinks: FC<WithCrossLinksProps> = ({ navKey }) => {

return (
<div className="mt-4 grid w-full grid-cols-2 gap-4 xs:grid-cols-1">
{(previousCrossLink && (
{previousCrossLink && (
Copy link
Member

Choose a reason for hiding this comment

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

Afaik the div element is intentional, otherwise the nextCrossLink goes to the left, instead of staying in the right 😅

<CrossLink
type="previous"
text={previousCrossLink.label}
link={previousCrossLink.link}
/>
)) || <div />}
)}

{nextCrossLink && (
<CrossLink
Expand Down
2 changes: 1 addition & 1 deletion util/detectOS.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { UserOS } from '@/types/userOS';

export const detectOsInUserAgent = (userAgent: string | undefined): UserOS => {
const osMatch = userAgent?.match(/(Win|Mac|Linux)/);
const osMatch = userAgent?.match(/(Win|Mac|Linux|AIX)/);
Copy link
Member

Choose a reason for hiding this comment

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

I think as mentioned before, instead of adding AIX to this match, we should remove the "case AIX"

switch (osMatch && osMatch[1]) {
case 'Win':
return 'WIN';
Expand Down