rolling-scopes / site

🦥🛼 Website supporting The Rolling Scopes and The Rolling Scopes School educational process
https://rs.school
12 stars 10 forks source link

Proposal: increase code consistency and readability #460

Open thirdmadman opened 2 months ago

thirdmadman commented 2 months ago

Work In Progress, not final

📝 Describe the refactoring task

Types, functions and variables sequentially declaration

I noticed that in some files, the types are not declared sequentially.

Suppose this can be clearly seen here

In the screenshot, it is underlined in red because I added to my eslint rules 'no-use-before-define': 'error',

This also partially concerns the declaration of functions - in one file.

Comments in the code

I see quite a lot of comments in the code. Some of them simply duplicate what is written in the code, for example:

function App() {
  // If we are on https://rollingscopes.com/, a redirect to https://rs.school/ is triggered.
  if (window.location.hostname.includes('rollingscopes.com')) {
    window.location.href = 'https://rs.school';
  }

Some of them explain something (which might need to be corrected):

export type EventCardProps = {
  eventType: string;
  title: string;
  organizedBy: string; // organizer name or place (e.g. 'Vilnius, Lithuania' or 'online')

And some of them are "to do," for example:

export const MentorsWanted = () => {
  // todo use custom link
  return (
    <section className={cx('mentors-wanted', 'container')}>
      <article className={classNames('content', cx('content'))}>
        <div className={cx('content-left')}>
          <WidgetTitle mods="lines">Mentors Wanted!</WidgetTitle>

And there is one fixme:

      sizes={isImageSvg ? undefined : sizes}
      decoding={decoding}
      // FIXME: remove this line when fetchPriority prop will be fixed
      // see https://github.com/facebook/react/issues/27233#issuecomment-2035176576

Component props types inconsistency

I also noticed that some of the parameters that are passed to React components are typed using interface, and some using type. Yes, I understand that somewhere this is done on purpose, otherwise it won’t work, I’m talking about cases where you can use both type and interface.

Descriptive consistent names

For example, I wonder if it's normal to shorten words like this?

export type LoadingAttr = ImgHTMLAttributes<HTMLImageElement>['loading'];

export type FetchPriorityAttr = ImgHTMLAttributes<HTMLImageElement>['fetchPriority'];

export type DecodingAttr = ImgHTMLAttributes<HTMLImageElement>['decoding'];

I'm also interested in the logic of naming components. For example, there is TextWithLink, which actually returns JSX.Element[]. It turns out that this is not just one "Text" but a whole array.

interface TextWithLinkProps {
  data: LinkList;
}

export const TextWithLink = ({ data }: TextWithLinkProps) => {
  return data.map(({ id, text, link, title }) => (
    <Fragment key={id}>
      {text && <span>{text}</span>}
      <Link className="required-link" to={link}>
        {title}
      </Link>
    </Fragment>
  ));
};

Or, for example, the Logo component. Is this any logo at all, or is it specifically RsSchoolLogo?

import { HTMLAttributes } from 'react';
import { VariantProps, cva } from 'class-variance-authority';
import classNames from 'classnames/bind';
import logo from '@/shared/assets/svg/rss-logo.svg';
import Image from '@/shared/ui/image';

import styles from './logo.module.scss';

type LogoProps = Pick<HTMLAttributes<HTMLElement>, 'className'> & VariantProps<typeof logoVariants>;

export const cx = classNames.bind(styles);

const logoVariants = cva(cx('logo'), { variants: { type: { 'with-border': cx('with-border') } } });

export const Logo = ({ type, className }: LogoProps) => {
  return (
    <div
      className={logoVariants({
        type,
        className,
      })}
      data-testid="logo"
    >
      <Image
        src={logo}
        alt="RSS-logo"
      />
    </div>
  );
};

And what is the SocialMedia component? If you read the code, it turns out that it is SocialMediaLink (and if you look at it in more detail, even SocialMediaLinkWithIcon).

import { ReactNode } from 'react';

import './social-media.scss';

export type SocialMediaProps = {
  title: string;
  href: string;
  icon: ReactNode;
};

export const SocialMedia = ({ title, href, icon }: SocialMediaProps) => (
  <a className="social-media" href={href} target="_blank" rel="noreferrer">
    {icon}
    <span className="text">{title}</span>
  </a>
);

Some naming standards have to be applied.

📍 Code location

Many locations.

🛠️ Suggested improvement

Types, functions and variables sequentially declaration

Types, functions, and variables have to be declared sequentially. We can use the eslint rule 'no-use-before-define': 'error', to enhance this in code.

Comments in the code

Comments in the code have to be banned. Reasons:

There should be an exception for this rule. Some comments can be critical. This exception should formulated and described.

Possibly we can use some rule for eslint to ban comments and prefix like // IMPORTANT: for exeptions.

Component props types inconsistency

We have to use interfaces to type or props, and in case we can not use interface, type has to be used.

Descriptive consistent names

We have to name our variables, functions, classes, and types according to some set of rules.

Exampels:

AboutList -> AboutCommunityMenu DesktopView -> FooterDesktopContent NavItem -> NavigationItem BurgerMenu -> BurgerMenuButton Partnered -> PartneresSection General -> CoursesGeneralInformationSection Main -> PageHeadingSection Certification -> CertificationSection Courses -> AllCoursesSection

ℹ️ Additional context

Add any other context, like the effects on the existing code or future development, here.

✅ Definition of Done (DoD):

Provide clear criteria that signify the successful implementation of this refactoring issue.

🕰️ Timeline:

Provide the deadline or estimated timeframe for the bug fix.

thirdmadman commented 2 months ago

https://andreferrazdev.medium.com/a-naming-convention-for-ui-components-77f4fb8797c

thirdmadman commented 2 months ago

Work In progress:

find ./src/ -type f \( -name "*.tsx" -o -name "*.ts" \) -exec grep -E "export const [A-Z]" {} +

./src/app/layouts/base-layout/base-layout.tsx: export const BaseLayout = () => {

./src/app/layouts/base-layout/components/footer/about-list.tsx: export const AboutList = () => { export const AboutCommunityMenu = () => {

./src/app/layouts/base-layout/components/footer/copyright.tsx: export const Copyright = () => {

./src/app/layouts/base-layout/components/footer/desktop-view.tsx: export const DesktopView = () => { export const FooterDesktopContent = () => {

./src/app/layouts/base-layout/components/footer/footer.tsx: export const Footer = () => {

./src/app/layouts/base-layout/components/header/burger/burger.tsx: export const BurgerMenu = ({ isMenuOpen, toggleMenu }: BurgerProps) => { export const BurgerMenuButton = ({ isMenuOpen, toggleMenu }: BurgerMenuButtonProps) => {

./src/app/layouts/base-layout/components/header/dropdown/dropdown-wrapper.tsx: export const DropdownWrapper = ({

./src/app/layouts/base-layout/components/header/header.tsx: export const Header = () => {

./src/app/layouts/base-layout/components/header/nav-item/nav-item.tsx: export const NavItem = ({ label, href, dropdownInner }: NavItemProps) => { export const NavigationItem = ({ label, href, dropdownInner }: NavigationItemProps) => {

./src/app/layouts/base-layout/components/partnered/partnered.tsx: export const Partnered = () => ( export const PartneresSection = () => (

./src/app/layouts/base-layout/components/scroll-to-hash/scroll-to-hash.tsx: export const ScrollToHashElement = () => {

./src/entities/courses/ui/course-card/course-card.tsx: export const CourseCard = ({

./src/entities/courses/ui/general/general.tsx: export const General = () => { export const CoursesGeneralInformationSection = () => {

./src/entities/courses/ui/main/main.tsx: export const Main = () => { export const PageHeadingSection = () => {

./src/entities/events/ui/event-card/event-card.tsx: export const EventCard = ({

./src/pages/angular.tsx: export const Angular = () => {

./src/pages/aws-developer.tsx: export const AwsDeveloper = () => {

./src/pages/aws-fundamentals.tsx: export const AwsFundamentals = () => {

./src/pages/community.tsx: export const Community: FC = () => {

./src/pages/courses.tsx: export const Courses: FC = () => {

./src/pages/home.tsx: export const Home: FC = () => {

./src/pages/javascript-en.tsx: export const JavaScriptEn = () => {

./src/pages/javascript-preschool-ru.tsx: export const JavaScriptPreSchoolRu = () => {

./src/pages/javascript-ru.tsx: export const JavaScriptRu = () => {

./src/pages/nodejs.tsx: export const Nodejs: FC = () => {

./src/pages/not-found.tsx: export const NotFound = () => ;

./src/pages/react.tsx: export const React = () => {

./src/shared/icons/angular-icon.tsx: export const AngularIcon = () => {

./src/shared/icons/arrow-icon.tsx: export const ArrowIcon = ({ size = '24px' }: { size?: string }) => {

./src/shared/icons/aws.tsx: export const AwsLogo = () => {

./src/shared/icons/discord-logo.tsx: export const DiscordLogo = () => {

./src/shared/icons/dropdown-arrow.tsx: export const DropdownArrow = () => {

./src/shared/icons/email.tsx: export const EmailIcon = () => {

./src/shared/icons/epam.tsx: export const EpamLogo = () => {

./src/shared/icons/facebook.tsx: export const FacebookIcon = () => {

./src/shared/icons/github.tsx: export const GithubLogo = () => {

./src/shared/icons/html-icon.tsx: export const HtmlIcon = () => {

./src/shared/icons/instagram.tsx: export const InstagramIcon = () => {

./src/shared/icons/javascript-icon.tsx: export const JavascriptIcon = () => {

./src/shared/icons/jetbrains.tsx: export const JetBrainsLogo = () => {

./src/shared/icons/linkedIn.tsx: export const LinkedInIcon = () => {

./src/shared/icons/nodejs-icon.tsx: export const NodeJsIcon = () => {

./src/shared/icons/open-source-philosophy-icon.tsx: export const OpenSourcePhilosophyIcon = () => {

./src/shared/icons/open-to-everyone-icon.tsx: export const OpenToEveryoneIcon = () => {

./src/shared/icons/react-icon.tsx: export const ReactIcon = () => {

./src/shared/icons/rs-banner.tsx: export const RsBanner = () => {

./src/shared/icons/teach-It-forward-icon.tsx: export const TeachItForwardIcon = () => {

./src/shared/icons/telegram.tsx: export const TelegramIcon = () => {

./src/shared/icons/text-link-icon.tsx: export const TextLinkIcon = () => {

./src/shared/icons/youtube.tsx: export const YouTubeIcon = () => {

./src/shared/ui/actions/actions.tsx: export const Actions = ({ actions, marked = false }: ActionsProps) => {

./src/shared/ui/date-lang/date-lang.tsx: export const DateLang = ({ startDate, language, mode, withMargin }: DateLangProps) => {

./src/shared/ui/image/image.tsx: export const Image = ({ alt, src = '', lazy = true, ...props }: ImageProps) => {

./src/shared/ui/link-custom/link-custom.tsx: export const LinkCustom = ({

./src/shared/ui/logo/logo.tsx: export const Logo = ({ type, className }: LogoProps) => {

./src/shared/ui/main-title/main-title.tsx: export const MainTitle = ({ children, className }: MainTitleProps) => {

./src/shared/ui/paragraph/paragraph.tsx: export const Paragraph = ({ children }: ParagraphProps) => (

./src/shared/ui/section-label/section-label.tsx: export const SectionLabel = ({ children, className, ...props }: SectionLabelProps) => {

./src/shared/ui/social-media/social-media.tsx: export const SocialMedia = ({ title, href, icon }: SocialMediaProps) => (

./src/shared/ui/subtitle/subtitle.tsx: export const Subtitle = ({ text, type = '' }: SubtitleProps) => (

./src/shared/ui/text-with-link/text-with-link.tsx: export const TextWithLink = ({ data }: TextWithLinkProps) => {

./src/shared/ui/widget-title/widget-title.tsx: export const WidgetTitle = ({ children, size, mods, className }: WidgetTitleProps) => {

./src/widgets/about/ui/about.tsx: export const About = ({ courseName, type = 'en' }: AboutProps) => {

./src/widgets/about/ui/info-grid/info-grid.tsx: export const InfoGrid = ({ items }: InfoGridProps) => {

./src/widgets/about-home/ui/about.tsx: export const About = () => {

./src/widgets/about-school/ui/about.tsx: export const About = () => {

./src/widgets/about-video/ui/about-video.tsx: export const AboutVideo = ({ lang = 'en' }: AboutVideoProps) => {

./src/widgets/alumni/ui/alumni.tsx: export const Alumni = () => {

./src/widgets/angular-topics/ui/angular-topics.tsx: export const AngularTopics = () => {

./src/widgets/breadcrumbs/ui/breadcrumbs.tsx: export const Breadcrumbs = () => {

./src/widgets/certification/ui/certification.tsx: export const Certification = ({ courseName }: RequiredProps) => { export const CertificationSection = ({ courseName }: RequiredProps) => {

./src/widgets/communication/ui/communication.tsx: export const Communication = ({ courseName, lang = 'en' }: RequiredProps) => {

./src/widgets/community/ui/community.tsx: export const Community = () => (

./src/widgets/contribute/ui/contribute.tsx: export const Contribute = () => (

./src/widgets/course-main/course-main.tsx: export const CourseMain = ({ courseName, lang = 'en', type }: CourseMainProps) => {

./src/widgets/courses/ui/courses.tsx: export const Courses = () => { export const AllCoursesSection = () => {

./src/widgets/courses-school/ui/CourseCard.tsx: export const CourseCard = ({

./src/widgets/courses-school/ui/courses.tsx: export const UpcomingCoursesSection = () => {

./src/widgets/events/ui/events.tsx: export const Events = () => { export const CommunityEventsSection = () => {

./src/widgets/faq/ui/faq.tsx: export const Faq = () => { export const JsFePreSchoolFaqSection = () => {

./src/widgets/hero/ui/hero.tsx: export const Hero = () => { export const CommunityHeroSection = () => {

./src/widgets/main/ui/main.tsx: export const Main = () => { export const HomeHeroSection = () => {

./src/widgets/mentoring/ui/mentoring.tsx: export const Mentoring = () => { export const HomeMentoringSection = () => {

./src/widgets/mentors/ui/mentors.tsx: export const Mentors = () => { export const HomeMentorsWantedSection = () => {

./src/widgets/mentors-wanted/ui/mentors-wanted.tsx: export const MentorsWanted = () => { export const AngularMentorsWantedSection = () => {

./src/widgets/merch/ui/merch.tsx: export const Merch = () => ( export const CommunityMerchSection = () => (

./src/widgets/mobile-view/ui/mobile-view.tsx: export const MobileView = ({ type }: MobileViewProps) => { export const FooterMobileContent = ({ type }: FooterMobileContentProps) => {

./src/widgets/not-found/ui/not-found.tsx: export const NotFound = () => { export const NotFoundPageContent = () => {

./src/widgets/numbers/ui/numbers.tsx: export const Numbers = () => { export const CommunityWeInNumbersSection = () => {

./src/widgets/option-item/ui/option-item.tsx: export const OptionItem = ({ title, description, linkLabel, href = '' }: OptionItemProps) => (

./src/widgets/pictures/ui/pictures.tsx: export const Pictures = () => ( export const CommunityWeInPicturesSection = () => (

./src/widgets/places/ui/places.tsx: export const Places = () => ( export const CommunityLocationsSection = () => (

./src/widgets/principles/ui/principle-card/principle-card.tsx: export const PrincipleCard = ({ title, description, icon }: PrincipleCardProps) => ( export const OurPrinciplesCard = ({ title, description, icon }: OurPrinciplesCardProps) => (

./src/widgets/principles/ui/principle-card/principles.tsx: export const Principles = () => ( export const HomeOurPrinciplesSection = () => (

./src/widgets/required/ui/required.tsx: export const Required = ({ courseName, marked1, marked2 }: RequiredProps) => { export const CourseRequiredKnowledgeSection = ({ courseName, marked1, marked2 }: CourseRequiredKnowledgeSectionProps) => {

./src/widgets/requirements/ui/requirements.tsx: export const Requirements = () => { export const HomeMentorRequirementsSection = () => {

./src/widgets/school-menu/ui/school-item/school-item.tsx: export const SchoolItem = ({ item, color }: SchoolItemProps) => {

./src/widgets/school-menu/ui/school-list/school-list.tsx: export const SchoolList = ({ list, color }: SchoolListProps) => {

./src/widgets/school-menu/ui/school-menu.tsx: export const SchoolMenu = ({ heading, hasTitle = true, color = 'light' }: SchoolMenuProps) => {

./src/widgets/speakers/ui/speakers.tsx: export const Speakers = () => (

./src/widgets/study-path/ui/stage-card/actions/actions.tsx: export const Actions = ({ actions, marked = false }: ActionsProps) => {

./src/widgets/study-path/ui/stage-card/image/image.tsx: export const Image = ({ imageSrc, title }: ImageProps) => {

./src/widgets/study-path/ui/stage-card/links/links.tsx: export const Links = ({ links }: LinksProps) => {

./src/widgets/study-path/ui/stage-card/logo-icon/logo-icon.tsx: export const LogoIcon = ({ icon, title }: LogoIconProps) => {

./src/widgets/study-path/ui/stage-card/stage-card.tsx: export const StageCard = ({

./src/widgets/study-path/ui/stage-card/step/step.tsx: export const Step = ({ id }: StepProps) => {

./src/widgets/study-path/ui/stage-card/topics/topics.tsx: export const Topics = ({ topics }: TopicsProps) => {

./src/widgets/study-path/ui/stages/stages.tsx: export const Stages = ({ stages, marked }: StagesProps) => {

./src/widgets/study-path/ui/study-path.tsx: export const LangContext = createContext<'ru' | 'en'>('en');

./src/widgets/study-path/ui/study-path.tsx: export const StudyPath = ({ path, marked, lang = 'en' }: StudyPathProps) => {

./src/widgets/study-with-us/ui/study-with-us.tsx: export const StudyWithUs = () => (

./src/widgets/support/ui/support.tsx: export const Support = () => (

./src/widgets/trainers/ui/trainers-card/trainer-card.tsx: export const TrainerCard = ({ name, bio, role, photo }: TrainerProps) => {

./src/widgets/trainers/ui/trainers.tsx: export const Trainers = ({ trainers, lang = 'en' }: TrainersProps) => {

./src/widgets/training-program/ui/training-program.tsx: export const TrainingProgram = ({ courseName, lang = 'en' }: TrainingProgramProps) => {