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

566-refactor: Fsd widget study path #772

Draft
wants to merge 83 commits into
base: main
Choose a base branch
from

Conversation

ansivgit
Copy link
Collaborator

@ansivgit ansivgit commented Feb 2, 2025

What type of PR is this? (select all that apply)

  • πŸ• Feature
  • πŸ› Bug Fix
  • 🚧 Breaking Change
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ“ Documentation Update

Description

Refactor due FSD

Related Tickets & Documents

Added/updated tests?

  • πŸ‘Œ Yes
  • πŸ™…β€β™‚οΈ No, because they aren't needed
  • πŸ™‹β€β™‚οΈ No, because I need help

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@ansivgit ansivgit self-assigned this Feb 2, 2025
@github-actions github-actions bot changed the title Refactor/566 widget study path 566-refactor: Fsd widget study path Feb 2, 2025
@ansivgit ansivgit added draft PR draft and removed preview labels Feb 2, 2025
@ansivgit ansivgit changed the title 566-refactor: Fsd widget study path DRAFT: 566-refactor: Fsd widget study path Feb 2, 2025
@ansivgit ansivgit added refactor preview and removed draft PR draft labels Feb 20, 2025
@ansivgit ansivgit changed the title DRAFT: 566-refactor: Fsd widget study path 566-refactor: Fsd widget study path Feb 20, 2025
Copy link

Lighthouse Report:

  • Performance: 77 / 100
  • Accessibility: 100 / 100
  • Best Practices: 100 / 100
  • SEO: 100 / 100

View detailed report

import { Stages } from '../stages/stages';
import { Paragraph } from '@/shared/ui/paragraph';
import { WidgetTitle } from '@/shared/ui/widget-title';
import type { StagePathPage } from '@/widgets/study-path/types';
Copy link
Collaborator

Choose a reason for hiding this comment

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

In our other components, we define the component's prop types within the component file itself

@@ -0,0 +1,23 @@
import cn from 'classnames';
Copy link
Collaborator

Choose a reason for hiding this comment

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

import classNames from 'classnames/bind';
...
const cx = classNames.bind(styles);

Comment on lines +5 to +7
export type StagePathPage = {
page: 'courses' | 'jsEn' | 'jsRu' | 'angular' | 'awsDev';
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd define a separate type for the page property (e.g., type PageType = ...). Sinсe this might improve reusability in other parts of the codebase (e.g. the test file).
And about the naming, I think using js-en instead of jsEn would be better.

import feJsStage2 from '@/shared/assets/stages/stage-2.webp';
import feJsStage3 from '@/shared/assets/stages/stage-3.webp';

export const studyPath = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a type for the studyPath object?

};
};

export type Module = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd either remove the export or rename the type, because the current name is too generic

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we use this file to export the types as well?

const stageStep = screen.getByTestId('stage-step');

expect(stageStep).toBeVisible();
expect(stageStep.innerHTML).toBe(mockProps.id.toString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could you expect(stageStep).toHaveTextContent(mockProps.id.toString());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FSD: Widget StudyPath
6 participants