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

fix(i18n): add missing translation keys for all locales #7711

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phungmanhcuong
Copy link
Contributor

Summary

This pull request addresses missing translation keys across all supported locales, ensuring consistency and preventing fallback issues.

Changes Made

  • Added missing keys to locale files for modules such as course, assessment, and submission.
  • Verified alignment between locales to ensure completeness.
  • Improved the overall quality and reliability of the internationalization (i18n) setup.

Impact

  • Enhances the localization experience for all supported languages.
  • Prevents incomplete translations and improves the user experience in non-default locales.

@phungmanhcuong phungmanhcuong self-assigned this Dec 16, 2024
@phungmanhcuong phungmanhcuong force-pushed the mcphung/add-missing-translation branch from a4927ae to 601fb15 Compare December 17, 2024 01:32
Copy link
Contributor

@cysjonathan cysjonathan left a comment

Choose a reason for hiding this comment

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

By the way, your current PR state has a broken leaderboard page

@@ -1,5 +1,5 @@
import { FC, memo, ReactNode, useMemo, useState } from 'react';
import { injectIntl, WrappedComponentProps } from 'react-intl';
import { defineMessages, injectIntl, WrappedComponentProps } from 'react-intl';
Copy link
Contributor

Choose a reason for hiding this comment

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

import { FC, memo, ReactNode, useMemo, useState } from 'react';
import { defineMessages, injectIntl, WrappedComponentProps } from 'react-intl';
import {
  ArrowDropDown as ArrowDropDownIcon,
  ArrowDropUp as ArrowDropUpIcon,
} from '@mui/icons-material';
import {
  Button,
  TableBody,
  TableCell,
  TableHead,
  TableRow,
} from '@mui/material';
import equal from 'fast-deep-equal';
import {
  FolderMiniEntity,
  MaterialMiniEntity,
} from 'types/course/material/folders';

import TableContainer from 'lib/components/core/layouts/TableContainer';
import useTranslation, { Descriptor } from 'lib/hooks/useTranslation';

import TableMaterialRow from './TableMaterialRow';
import TableSubfolderRow from './TableSubfolderRow';

interface Props extends WrappedComponentProps {
  currFolderId: number;
  subfolders: FolderMiniEntity[];
  materials: MaterialMiniEntity[];
  isCurrentCourseStudent: boolean;
  isConcrete: boolean;
}

const translations = defineMessages({
  name: {
    id: 'course.material.folders.WorkbinTable.name',
    defaultMessage: 'Name',
  },
  lastModified: {
    id: 'course.material.folders.WorkbinTable.lastModified',
    defaultMessage: 'Last Modified',
  },
  startAt: {
    id: 'course.material.folders.WorkbinTable.startAt',
    defaultMessage: 'Start At',
  },
});

const WorkbinTable: FC<Props> = (props) => {
  const { t } = useTranslation();

  const {
    currFolderId,
    subfolders,
    materials,
    isCurrentCourseStudent,
    isConcrete,
  } = props;

  const [sortBy, setSortBy] = useState<Descriptor>(translations.name);
  const [sortDirection, setSortDirection] = useState<'asc' | 'desc'>('asc');

  const sortedSubfolders = useMemo(() => {
    return subfolders.sort((a, b) => {
      switch (sortBy) {
        case translations.name:
          if (sortDirection === 'asc') {
            return a.name.toUpperCase() > b.name.toUpperCase() ? 1 : -1;
          }
          return a.name.toUpperCase() > b.name.toUpperCase() ? -1 : 1;

        case translations.startAt:
          if (sortDirection === 'asc') {
            return a.startAt > b.startAt ? 1 : -1;
          }
          return a.startAt > b.startAt ? -1 : 1;

        case translations.lastModified:
          if (sortDirection === 'asc') {
            return a.updatedAt > b.updatedAt ? 1 : -1;
          }
          return a.updatedAt > b.updatedAt ? -1 : 1;

        default:
          return 0;
      }
    });
  }, [subfolders, sortBy, sortDirection]);

  const sortedMaterials = useMemo(() => {
    return materials.sort((a, b) => {
      switch (sortBy) {
        case translations.name:
          if (sortDirection === 'asc') {
            return a.name.toUpperCase() > b.name.toUpperCase() ? 1 : -1;
          }
          return a.name.toUpperCase() > b.name.toUpperCase() ? -1 : 1;

        case translations.startAt:
          if (sortDirection === 'asc') {
            return a.updatedAt > b.updatedAt ? 1 : -1;
          }
          return a.updatedAt > b.updatedAt ? -1 : 1;

        default:
          return 0;
      }
    });
  }, [materials, sortBy, sortDirection]);

  const sort = (columnName: Descriptor): void => {
    if (columnName === sortBy) {
      if (sortDirection === 'asc') {
        setSortDirection('desc');
      } else {
        setSortDirection('asc');
      }
    } else {
      setSortBy(columnName);
      setSortDirection('asc');
    }
  };

  const columnHeaderWithSort = (columnName: Descriptor): JSX.Element => {
    let endIcon: ReactNode = null;
    if (sortBy === columnName && sortDirection === 'desc') {
      endIcon = <ArrowDropDownIcon />;
    } else if (sortBy === columnName && sortDirection === 'asc') {
      endIcon = <ArrowDropUpIcon />;
    }

    return (
      <Button
        disableFocusRipple
        disableRipple
        endIcon={endIcon}
        onClick={(): void => {
          sort(columnName);
        }}
        style={{ padding: 0, alignItems: 'center', justifyContent: 'start' }}
      >
        {t(columnName)}
      </Button>
    );
  };

  return (
    <TableContainer dense variant="bare">
      <TableHead>
        <TableRow>
          <TableCell>{columnHeaderWithSort(translations.name)}</TableCell>
          <TableCell>
            {columnHeaderWithSort(translations.lastModified)}
          </TableCell>
          {!isCurrentCourseStudent && (
            <TableCell>{columnHeaderWithSort(translations.startAt)}</TableCell>
          )}
          <TableCell />
        </TableRow>
      </TableHead>
      <TableBody>
        {sortedSubfolders.map((subfolder) => {
          return (
            <TableSubfolderRow
              key={`subfolder-${subfolder.id}`}
              currFolderId={currFolderId}
              isConcrete={isConcrete}
              isCurrentCourseStudent={isCurrentCourseStudent}
              subfolder={subfolder}
            />
          );
        })}
        {sortedMaterials.map((material) => {
          return (
            <TableMaterialRow
              key={`material-${material.id}`}
              currFolderId={currFolderId}
              isConcrete={isConcrete}
              isCurrentCourseStudent={isCurrentCourseStudent}
              material={material}
            />
          );
        })}
      </TableBody>
    </TableContainer>
  );
};

export default memo(injectIntl(WorkbinTable), (prevProps, nextProps) => {
  return equal(prevProps, nextProps);
});

You can do something like this, then you won't need to modify DataTable.jsx.

You may need to allow DataTable.ts label to accept Descriptor type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reverted the changes on DataTable.jsx and updated its usage places one step at a time.
However, I found that there is inconsistency in translating messages; some places use formatMessage, while others use useTranslation.

Copy link
Contributor

Choose a reason for hiding this comment

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

The places which use formatMessage are older code before the useTranslation hook was created, For newer translations, we should use useTranslation where possible.

@phungmanhcuong phungmanhcuong force-pushed the mcphung/add-missing-translation branch 2 times, most recently from 03d38e1 to 7abab7c Compare December 17, 2024 08:37
@@ -42,6 +66,7 @@ const styles = {

const AchievementTable: FC<Props> = (props) => {
const { achievements, permissions, onTogglePublished, isReordering } = props;
const { formatMessage: t } = useIntl();
Copy link
Contributor

Choose a reason for hiding this comment

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

We have const { t } = useTranslation(); for this?

Comment on lines 40 to 43
const translateStatus: (var1: string) => {
id: string;
defaultMessage: string;
} = (oldStatus) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Descriptor type?

@phungmanhcuong phungmanhcuong force-pushed the mcphung/add-missing-translation branch 3 times, most recently from dae4ea5 to 8424359 Compare December 18, 2024 01:59
@@ -18,7 +18,9 @@
max-width: 144px;
min-width: 100px;
padding: 12px;
transition: background-color 0.5s, color 0.5s;
transition:
Copy link

Choose a reason for hiding this comment

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

Colon after property should be followed by one space

@phungmanhcuong phungmanhcuong force-pushed the mcphung/add-missing-translation branch 2 times, most recently from 78d66f3 to e1cc3a2 Compare December 18, 2024 05:48
- Added missing translation keys for Chinese and Korean locales.
- Updated components to address missing translations in several places.

This ensures improved localization coverage and consistency.
@phungmanhcuong phungmanhcuong force-pushed the mcphung/add-missing-translation branch from e1cc3a2 to 2f422e1 Compare December 18, 2024 08:46
@phungmanhcuong
Copy link
Contributor Author

@cysjonathan the PR has done and please take a look.

const AchievementAwardSummary: FC<Props> = (props) => {
const { achievementUsers, initialObtainedUserIds, selectedUserIds } = props;
const { formatMessage: t } = useIntl();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't you using the useTranslation hook?

@@ -1,5 +1,5 @@
import { FC, memo, ReactNode, useMemo, useState } from 'react';
import { injectIntl, WrappedComponentProps } from 'react-intl';
import { defineMessages, injectIntl, WrappedComponentProps } from 'react-intl';
Copy link
Contributor

Choose a reason for hiding this comment

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

The places which use formatMessage are older code before the useTranslation hook was created, For newer translations, we should use useTranslation where possible.

});

const VideoTable: FC<Props> = (props) => {
const { videos, permissions, onTogglePublished, intl } = props;
const { formatMessage: t } = intl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Update?

@@ -1,5 +1,5 @@
import { FC, memo } from 'react';
import { defineMessages, FormattedMessage } from 'react-intl';
import { defineMessages, FormattedMessage, useIntl } from 'react-intl';
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused useIntl import?

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

Successfully merging this pull request may close these issues.

2 participants