Standardize Dialog Components, handle zero width spaces in common blocks, fix flip cards in Firefox #1368
Conversation
- Added `removeNonBreakingSpaces` utility to clean up uncommon space characters in paragraph blocks. - Updated `PageEditor` to utilize the new utility when normalizing document content. - Introduced unit tests for `removeNonBreakingSpaces` to ensure functionality across various block types and nested structures.
WalkthroughThis pull request introduces a comprehensive set of changes across multiple services and components, focusing on standardizing dialog implementations, enhancing form interactions, and improving localization. The primary themes include replacing custom Changes
Sequence DiagramsequenceDiagram
participant User
participant UI
participant StandardDialog
participant Form
participant Backend
User->>UI: Trigger Action
UI->>StandardDialog: Open Dialog
StandardDialog->>Form: Render Form
User->>Form: Fill Form
User->>Form: Submit
Form->>Backend: Submit Data
Backend-->>Form: Response
Form-->>StandardDialog: Close Dialog
StandardDialog-->>UI: Update UI
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
quizzes
shared-module
system-tests
course-material
cms
main-frontend
labels
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (4)
services/course-material/src/components/ContentRenderer/moocfi/FlipCard/FlipCardBlock.tsx (1)
Line range hint
137-146: Add type safety to size handling.The
sizeStringToSizepxfunction should handle all possible size values and provide a default case.-function sizeStringToSizepx(block: Block<FlipCardAttributes>) { +function sizeStringToSizepx(block: Block<FlipCardAttributes>): number { + const DEFAULT_SIZE = 400; // m size as default if (block.attributes.size == "xl") { return 500 } else if (block.attributes.size == "m") { return 400 } else if (block.attributes.size == "s") { return 300 } + return DEFAULT_SIZE; }services/course-material/src/components/ContentRenderer/moocfi/FlipCard/InnerCardBlock.tsx (1)
Line range hint
52-57: Consider using absolute positioning instead of fixed.Using
position: fixedfor the button container could cause layout issues:
- The button might overlap with other content on small screens
- Fixed positioning relative to viewport may not work well when the component is embedded
- Scrolling behavior might be unexpected
Consider this alternative:
<div className={css` - position: fixed; + position: absolute; bottom: 5px; right: 5px; `} >services/main-frontend/src/components/forms/NewReferenceForm.tsx (1)
Line range hint
14-19: Remove unusedonCancelprop from interface.The
onCancelprop is defined in the interface but is no longer used after the removal of the cancel button.interface NewReferenceFormProps { onCreateNewReference: (form: NewMaterialReference[]) => void - onCancel: () => void }services/main-frontend/src/components/forms/EditExamForm.tsx (1)
Line range hint
15-20: Remove unusedonCancelprop from interface and component props.The
onCancelprop is defined but no longer used after the removal of the cancel button.interface EditExamFormProps { initialData: Exam organizationId: string onEditExam: (form: NewExam) => void - onCancel: () => void } const EditExamForm: React.FC<React.PropsWithChildren<EditExamFormProps>> = ({ initialData, onEditExam, - onCancel, organizationId, }) => {
🧹 Nitpick comments (28)
services/main-frontend/src/components/page-specific/manage/courses/id/references/EditReferenceDialog.tsx (1)
66-76: LGTM! Clean transition to StandardDialog.The implementation correctly uses the StandardDialog component while maintaining all necessary functionality.
Consider enhancing mutation feedback.
The mutation handlers could benefit from:
- Loading state handling to disable the form during mutations
- More specific success messages (e.g., including the reference title)
Example enhancement:
const updateReferenceMutation = useToastMutation( ... { notify: true, - successMessage: t("reference-updated-succesfully"), + successMessage: t("reference-updated-succesfully", { title: reference.title }), method: "POST", }, ... )services/main-frontend/src/components/page-specific/org/organizationSlug/CourseList.tsx (2)
73-73: Consider adding explicit error handling.While the current implementation works, consider adding try-catch blocks to handle potential errors and ensure the dialog stays open if the submission fails.
const handleSubmitNewCourse = async (newCourse: NewCourse) => { + try { await postNewCourse(newCourse) await getOrgCourses.refetch() await getOrgCourseCount.refetch() setNewCourseFormOpen(false) + } catch (error) { + // Handle error appropriately (e.g., show error message) + console.error('Failed to create course:', error) + } }Also applies to: 80-80
Line range hint
1-153: Great architectural improvement!The refactoring to use a standardized dialog component improves maintainability and consistency across the application. The separation of concerns between the course list management and dialog functionality is well-executed.
services/course-material/src/components/ContentRenderer/core/common/Image/ImageBlock.tsx (1)
58-60: Use optional chaining for safer access.The condition can be simplified using optional chaining.
-{linkTarget && linkTarget.includes("_blank") && ( +{linkTarget?.includes("_blank") && (🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
services/course-material/src/components/ContentRenderer/moocfi/FlipCard/FlipCardBlock.tsx (2)
66-72: Simplify the conditional transform logic.The transform condition can be simplified as both hover cases for flipped state return the same value.
-transform: ${isFlipped - ? isHovered - ? "rotateY(180deg)" - : "rotateY(180deg)" - : isHovered - ? "rotateY(10deg)" - : "rotateY(0)"}; +transform: ${isFlipped + ? "rotateY(180deg)" + : isHovered + ? "rotateY(10deg)" + : "rotateY(0)"};
97-103: Consider extracting common ContentRenderer props.The ContentRenderer component is used twice with identical props. Consider extracting these common props to reduce duplication.
const commonRendererProps = { editing: false, selectedBlockId: null, setEdits: function (): void {}, isExam: false, } // Usage: <ContentRenderer data={[frontCard]} {...commonRendererProps} /> <ContentRenderer data={[backCard]} {...commonRendererProps} />Also applies to: 123-129
services/cms/src/utils/Gutenberg/modifyBlocks.ts (1)
54-86: Consider refactoring the block type handling for better maintainability.The implementation is correct and immutable, but the switch statement could be refactored to be more maintainable.
Consider using a configuration object to map block types to their attributes:
+const BLOCK_ATTRIBUTES_MAP = { + 'core/paragraph': ['content'], + 'core/heading': ['content'], + 'moocfi/hero-section': ['title', 'subtitle'] +} export const removeUncommonSpacesFromBlocks = (blocks: BlockInstance[]): BlockInstance[] => { return blocks.map((block) => { const newBlock = { ...block } const attributes = { ...block.attributes } - switch (block.name) { - case "core/paragraph": - case "core/heading": - if (attributes.content) { - attributes.content = replaceUncommonSpaces(attributes.content) - } - break; - - case "moocfi/hero-section": - if (attributes.title) { - attributes.title = replaceUncommonSpaces(attributes.title) - } - if (attributes.subtitle) { - attributes.subtitle = replaceUncommonSpaces(attributes.subtitle) - } - break; - } + const attributesToProcess = BLOCK_ATTRIBUTES_MAP[block.name] || [] + attributesToProcess.forEach(attr => { + if (attributes[attr]) { + attributes[attr] = replaceUncommonSpaces(attributes[attr]) + } + }) newBlock.attributes = attributes if (block.innerBlocks && block.innerBlocks.length > 0) { newBlock.innerBlocks = removeUncommonSpacesFromBlocks(block.innerBlocks) } return newBlock }) }This approach:
- Makes it easier to add new block types
- Reduces code duplication
- Improves maintainability
services/cms/tests/utils/modifyBlocks.test.ts (1)
5-118: Add test case for multiple consecutive uncommon spaces.The test suite is comprehensive but could benefit from an additional test case.
Add this test case to verify handling of multiple consecutive uncommon spaces:
+ it("should handle multiple consecutive uncommon spaces", () => { + const blocks: BlockInstance[] = [ + { + name: "core/paragraph", + attributes: { + content: "Multiple\u00A0\u00A0\u2003Spaces\u2003\u2003Here", + }, + clientId: "1", + isValid: true, + innerBlocks: [], + }, + ] + + const result = removeUncommonSpacesFromBlocks(blocks) + + expect(result[0].attributes.content).toBe("Multiple Spaces Here") + expect(blocks[0].attributes.content).toBe("Multiple\u00A0\u00A0\u2003Spaces\u2003\u2003Here") + })services/cms/src/components/editors/PageEditor.tsx (1)
92-92: Document the content transformation pipeline.The order of operations in the content transformation pipeline should be documented.
Add a comment explaining the pipeline:
+ // Transform content pipeline: + // 1. Remove unsupported block types + // 2. Clean up uncommon spaces to improve localization content: removeUncommonSpacesFromBlocks(removeUnsupportedBlockType(content)),services/course-material/src/components/ContentRenderer/moocfi/FlipCard/FlipButton.tsx (2)
18-20: Remove redundant backface-visibility declaration.The
backface-visibility: hiddenproperty is duplicated. The child selector application is unnecessary since the parent's property will be inherited.- backface-visibility: hidden; - - & * { - backface-visibility: hidden; - } + backface-visibility: hidden;Also applies to: 16-16
24-33: Extract theme colors and improve hover state.Consider the following improvements:
- Extract hardcoded colors into theme variables
- Use a more specific color value for hover state instead of generic 'green'
- Consider adding a transition effect for smoother color changes
services/cms/src/blocks/FlipCard/BackFlipCard/BackFlipCardEditor.tsx (1)
20-24: Consider extracting commonly used styles.The styling implementation is correct, but since the AI summary indicates similar full-width styling is applied across multiple FlipCard components, consider extracting this into a shared style constant for better maintainability.
Example refactor:
+const fullWidthStyle = css` + width: 100%; +`; <div - className={css` - width: 100%; - `} + className={fullWidthStyle} >services/main-frontend/src/components/forms/NewCourseForm.tsx (5)
79-85: Automatically updating 'slug' may override user inputThe
slugfield is automatically updated whenever thenamechanges. If a user manually edits theslug, their input might be unintentionally overridden.Consider updating the
useEffectto only set theslugif it hasn't been manually modified:React.useEffect(() => { - if (name) { + const currentSlug = watch("slug") + if (name && (!currentSlug || currentSlug === normalizePath(previousName))) { setValue("slug", normalizePath(name)) } -}, [name, setValue]) +}, [name, setValue, watch])You'll need to keep track of the previous value of
nameto compare.
51-55: Integrate 'languageCodeValidationError' into form validationInstead of managing
languageCodeValidationErrorwithuseState, leveragereact-hook-form's validation methods for consistency.Here's how you can adjust the code:
const { register, handleSubmit, formState: { errors }, + setError, } = useForm<FormFields>() -const [languageCodeValidationError, setLanguageCodeValidationError] = React.useState<string | null>(null)In your custom language code field:
<TextField required label={t("language-code")} {...register("language_code", { validate: (value) => { try { normalizeIETFLanguageTag(value) return true } catch (e) { - setLanguageCodeValidationError(t("language-code-validation-error")) + return t("language-code-validation-error") } }, })} />Display the error message using
errors.language_code.
89-95: Simplify 'handleDuplicateMenu' functionThe function can be streamlined since you're only setting the
courseId.Apply this diff to simplify the function:
-const handleDuplicateMenu = (courseId: string) => { - const findCourse = courses?.find((course) => course.id === courseId) - if (findCourse?.name) { - setValue("courseId", courseId) - } -} +const handleDuplicateMenu = (courseId: string) => { + setValue("courseId", courseId) +}
227-232: Add a default value to the 'SelectField'Ensure the
SelectFieldreflects the initialcourseIdvalue fromdefaultValues.Modify the
SelectFieldcomponent as follows:<SelectField id="duplicate-course-select-menu" {...register("courseId")} + value={watch("courseId")} onChange={(e) => handleDuplicateMenu(e.target.value)} options={courses.map((course) => { return { label: course.name, value: course.id } })} />
314-318: Ensure accessibility by adding 'aria-label' to the submit buttonAdding an
aria-labelimproves accessibility for screen readers.<Button type="submit" variant="primary" size="medium" disabled={submitDisabled || !!languageCodeValidationError} fullWidth + aria-label={t("button-text-create")} > {t("button-text-create")} </Button>services/main-frontend/src/utils/normalizePath.ts (2)
2-8: Enhance Unicode character normalizationThe current implementation might not handle Unicode characters properly. Consider using
deburrfrom lodash to normalize accented characters.First, import
deburr:+import deburr from "lodash/deburr"Then, update the function:
export const normalizePath = (str: string): string => { return ( + deburr(str) .toLowerCase() .replace(/\s+/g, "-") .replace(/[^a-z0-9-]/g, "") .replace(/-+/g, "-") .replace(/^-|-$/g, "") ) }
2-8: Avoid disabling ESLint rules globallyDisabling ESLint rules should be limited to specific lines where necessary.
Modify the ESLint disable comment to target the specific line:
-/* eslint-disable i18next/no-literal-string */ +// eslint-disable-next-line i18next/no-literal-string export const normalizePath = (str: string): string => {services/main-frontend/src/components/page-specific/org/organizationSlug/NewCourseDialog.tsx (1)
9-16: Consider makingcoursesprop optional.Since the courses array is only used for the duplicate course functionality, consider making it optional to better reflect its usage pattern.
interface NewCourseDialogProps { open: boolean onClose: () => void organizationId: string - courses: Course[] + courses?: Course[] onSubmitNewCourse: (newCourse: NewCourse) => Promise<void> onSubmitDuplicateCourse: (oldCourseId: string, newCourse: NewCourse) => Promise<void> }services/main-frontend/src/components/page-specific/manage/courses/id/pages/ChapterFormDialog.tsx (1)
13-13: Consider updating the onSubmitForm type signature.The
onSubmitFormcallback should be typed to return a Promise to better handle asynchronous operations.interface ChapterFormDialogProps { open: boolean onClose: () => void courseId: string - onSubmitForm: () => void + onSubmitForm: () => Promise<void> chapterNumber: number initialData: Chapter | null newRecord: boolean }services/main-frontend/src/components/forms/UploadImageForm.tsx (1)
38-40: Consider enhancing the file input implementation.The button styling looks good, but consider these improvements:
- The
accept="image"attribute is too broad. Consider specifying exact MIME types (e.g., "image/jpeg,image/png").- Add file size validation before upload.
- Show a loading indicator during upload.
- accept="image" + accept="image/jpeg,image/png,image/gif"services/main-frontend/src/components/page-specific/manage/courses/id/code-giveaway/NewCodeGiveawayForm.tsx (2)
67-68: Remove unnecessary ESLint disable comment.The i18n ESLint disable comment seems unnecessary as the variant is a valid prop value, not a string literal that needs translation.
- // eslint-disable-next-line i18next/no-literal-string variant: "primary",
Line range hint
74-85: Enhance form validation and user feedback.Consider these improvements:
- Add validation for courseModuleId and requireCourseSpecificConsentFormQuestionId fields
- Provide more descriptive validation feedback to users
- Consider using a form library like Formik or react-hook-form for better form handling
shared-module/packages/common/src/components/StandardDialog.tsx (2)
129-143: Consider RTL support for button layout.The button container uses
justify-content: flex-endwhich might need adjustment for RTL languages.Consider using CSS logical properties:
- justify-content: flex-end; + justify-content: flex-start; + flex-direction: row-reverse;
25-147: Consider adding error boundary and animation support.To enhance robustness and user experience, consider:
- Wrapping dialog content in an error boundary
- Adding animation props for enter/exit transitions
Would you like me to provide an implementation for these enhancements?
services/main-frontend/src/components/page-specific/manage/courses/id/pages/NewOrEditPageForm.tsx (1)
92-104: Consider adding type safety for dialog buttons array.The buttons array could benefit from type safety to prevent potential runtime errors.
+ type DialogButton = { + disabled?: boolean; + variant: 'primary' | 'secondary'; + onClick: () => void; + children: React.ReactNode; + }; <StandardDialog open={open} onClose={onClose} title={isUpdate ? t("heading-edit-page") : t("heading-new-page")} - buttons={[ + buttons={[{ disabled: saveMutation.isPending, variant: "primary", onClick: handleSubmit, children: isUpdate ? t("button-text-update") : t("button-text-create"), - }]} + } as DialogButton]}services/main-frontend/src/components/page-specific/manage/courses/id/pages/PageList/PageAudioWidget.tsx (1)
108-114: Consider using theme variables for background color.The dialog implementation looks good, but consider using a theme variable for the background color instead of a hardcoded hex value to maintain consistency with the design system.
- backgroundColor="#ecf3f2" + backgroundColor={theme.colors.background.secondary}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
system-tests/src/__screenshots__/course-instance-management/course-instance-management.spec.ts/initial-management-page-editing-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/course-instance-management/course-instance-management.spec.ts/initial-management-page-editing-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/course-instance-management/course-instance-management.spec.ts/new-course-instance-form-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/course-instance-management/course-instance-management.spec.ts/new-course-instance-form-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/course-material/content-components-render.spec.ts/content-components-renderer-view-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/gutenberg/spacer-not-breaking-editing.spec.ts/new-block-inserter-should-not-obscure-typing-caret-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/material-reference.spec.ts/add-new-material-reference-dialog-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/material-reference.spec.ts/add-new-material-reference-dialog-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/material-reference.spec.ts/citation-in-editor-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/material-reference.spec.ts/material-reference-editor-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/material-reference.spec.ts/material-reference-editor-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/query-mutation-notifications/error-notification.spec.ts/error-notification-test-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/query-mutation-notifications/error-notification.spec.ts/error-notification-test-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/query-mutation-notifications/success-notification.spec.ts/success-notification-test-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/query-mutation-notifications/success-notification.spec.ts/success-notification-test-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/research-form.spec.ts/research-consent-form-shows-if-not-aswered-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/research-form.spec.ts/research-consent-form-shows-if-not-aswered-mobile-tall.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/sidebar-toggle.spec.ts/block-list-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/sidebar-toggle.spec.ts/block-menu-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/sidebar-toggle.spec.ts/block-properties-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/user-research-consent.spec.ts/research-consent-form-desktop-regular.pngis excluded by!**/*.pngsystem-tests/src/__screenshots__/user-research-consent.spec.ts/research-consent-form-mobile-tall.pngis excluded by!**/*.png
📒 Files selected for processing (50)
.stylelintrc(1 hunks)services/cms/src/blocks/FlipCard/BackFlipCard/BackFlipCardEditor.tsx(2 hunks)services/cms/src/blocks/FlipCard/FlipCardEditor.tsx(2 hunks)services/cms/src/blocks/FlipCard/FrontFlipCard/FrontFlipCardEditor.tsx(2 hunks)services/cms/src/components/editors/PageEditor.tsx(2 hunks)services/cms/src/styles/LocalStyles.tsx(1 hunks)services/cms/src/utils/Gutenberg/modifyBlocks.ts(2 hunks)services/cms/tests/utils/modifyBlocks.test.ts(1 hunks)services/course-material/src/components/ContentRenderer/core/common/Image/ImageBlock.tsx(5 hunks)services/course-material/src/components/ContentRenderer/core/common/Image/ImageInteractivityContext.tsx(1 hunks)services/course-material/src/components/ContentRenderer/moocfi/FlipCard/FlipButton.tsx(1 hunks)services/course-material/src/components/ContentRenderer/moocfi/FlipCard/FlipCardBlock.tsx(2 hunks)services/course-material/src/components/ContentRenderer/moocfi/FlipCard/InnerCardBlock.tsx(3 hunks)services/main-frontend/src/components/forms/EditExamForm.tsx(2 hunks)services/main-frontend/src/components/forms/EditReferenceForm.tsx(1 hunks)services/main-frontend/src/components/forms/NewCourseForm.tsx(12 hunks)services/main-frontend/src/components/forms/NewExamForm.tsx(2 hunks)services/main-frontend/src/components/forms/NewReferenceForm.tsx(1 hunks)services/main-frontend/src/components/forms/UploadImageForm.tsx(1 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/code-giveaway/NewCodeGiveawayForm.tsx(3 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/course-instances/NewCourseInstanceDialog.tsx(2 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/course-instances/NewCourseInstanceForm.tsx(1 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/exams/EditExamDialog.tsx(2 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/exams/NewExamDialog.tsx(2 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/index/ManageCourse.tsx(1 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/index/UpdateCourseForm.tsx(4 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/language-versions/NewCourseLanguageVersionDialog.tsx(2 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/pages/ChapterFormDialog.tsx(1 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/pages/ManageCourseStructure.tsx(3 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/pages/NewChapterForm.tsx(1 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/pages/NewOrEditPageForm.tsx(4 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/pages/PageList/PageAudioWidget.tsx(3 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/pages/PageList/PageList.tsx(1 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/pages/PageList/PageListItem.tsx(1 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/references/EditReferenceDialog.tsx(2 hunks)services/main-frontend/src/components/page-specific/manage/courses/id/references/NewReferenceDialog.tsx(2 hunks)services/main-frontend/src/components/page-specific/org/organizationSlug/CourseList.tsx(4 hunks)services/main-frontend/src/components/page-specific/org/organizationSlug/NewCourseDialog.tsx(1 hunks)services/main-frontend/src/utils/normalizePath.ts(1 hunks)services/quizzes/src/components/exercise-service-views/ExerciseEditor/QuizComponents/common/MessageDialog.tsx(1 hunks)shared-module/packages/common/src/components/Button.tsx(3 hunks)shared-module/packages/common/src/components/StandardDialog.tsx(1 hunks)shared-module/packages/common/src/locales/ar/main-frontend.json(5 hunks)shared-module/packages/common/src/locales/en/main-frontend.json(5 hunks)shared-module/packages/common/src/locales/fi/main-frontend.json(5 hunks)shared-module/packages/common/src/locales/uk/main-frontend.json(5 hunks)system-tests/src/tests/course-language-versions.spec.ts(1 hunks)system-tests/src/tests/draft.spec.ts(3 hunks)system-tests/src/tests/material-reference.spec.ts(2 hunks)system-tests/src/tests/quizzes/create-and-save-quizzes.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/main-frontend/src/components/page-specific/manage/courses/id/pages/NewChapterForm.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
services/course-material/src/components/ContentRenderer/core/common/Image/ImageBlock.tsx
[error] 58-58: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (61)
services/main-frontend/src/components/page-specific/manage/courses/id/references/EditReferenceDialog.tsx (1)
8-8: LGTM!The import of
StandardDialogfrom the shared module is correctly placed and follows the established module pattern.services/main-frontend/src/components/page-specific/org/organizationSlug/CourseList.tsx (4)
13-13: LGTM! Clean import changes.The import changes align well with the refactoring goal of replacing the custom Dialog with NewCourseDialog.
36-36: LGTM! Clear state management.The state variable is well-named and follows React conventions for managing dialog visibility.
132-139: LGTM! Well-structured dialog integration.The NewCourseDialog component is properly integrated with all necessary props and handlers.
147-149: LGTM! Well-implemented button with proper permissions.The button implementation includes appropriate permission checks and internationalization.
services/course-material/src/components/ContentRenderer/core/common/Image/ImageInteractivityContext.tsx (1)
1-11: Well-structured context implementation!The implementation follows React's context best practices with a clear interface, sensible defaults, and a custom hook for simplified consumption.
services/course-material/src/components/ContentRenderer/core/common/Image/ImageBlock.tsx (2)
42-62: Good extraction of image rendering logic!The
renderImagefunction nicely encapsulates the image rendering logic, improving code organization.🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
85-104: Verify the alignment logic for figures.The alignment CSS seems complex with multiple conditions. Consider testing various alignment scenarios to ensure proper rendering.
Please test the following alignment cases:
- Center alignment with and without captions
- Left alignment with surrounding text
- Right alignment with surrounding text
services/course-material/src/components/ContentRenderer/moocfi/FlipCard/FlipCardBlock.tsx (1)
48-54: Good keyboard accessibility implementation!The keyboard handling properly supports both Enter and Space keys, following accessibility best practices.
services/cms/src/utils/Gutenberg/modifyBlocks.ts (3)
4-4: LGTM! Comprehensive Unicode space handling.The regex pattern effectively covers all major Unicode space variants that could cause issues in content localization.
26-36: Well-documented and implemented helper function!The function is pure, focused, and well-documented with JSDoc comments.
38-53: Excellent documentation!The JSDoc comments are thorough, clearly explaining the function's purpose, behavior, and handled Unicode characters.
services/cms/src/components/editors/PageEditor.tsx (1)
21-21: LGTM! Clean import statement.The import is correctly formatted and grouped with related functionality.
services/main-frontend/src/components/page-specific/manage/courses/id/exams/NewExamDialog.tsx (3)
10-10: LGTM! Import of StandardDialog aligns with standardization effort.The import is correctly placed and follows the shared module pattern.
69-84: LGTM! Error handling and form integration are well-implemented.The error handling is comprehensive, and the form integration maintains a clear separation of concerns. The mutation handlers are properly connected.
68-85: Verify translation key exists for dialog title.The StandardDialog implementation looks clean and maintainable. Let's verify the translation key exists.
✅ Verification successful
Translation key 'new-exam' is properly implemented
The translation key exists in all language files (en, fi, uk, ar) under shared-module/packages/common/src/locales/*/main-frontend.json with appropriate translations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the 'new-exam' translation key exists in translation files rg -g '*.json' '"new-exam"' services/main-frontend/src/translations/Length of output: 151
Script:
#!/bin/bash # First, find potential translation files fd -e json # Then specifically search for 'new-exam' in all JSON files rg -g '*.json' '"new-exam"'Length of output: 4238
services/main-frontend/src/components/page-specific/manage/courses/id/references/NewReferenceDialog.tsx (3)
8-8: LGTM! Import change aligns with standardization effort.The replacement of the Dialog import with StandardDialog follows the project's standardization initiative.
41-46: Clean implementation with proper component composition!The StandardDialog implementation is well-structured:
- Props are appropriately passed
- Child component integration is maintained
- Event handling remains consistent
41-46: Verify StandardDialog accessibility features.While the implementation is clean, we should verify that StandardDialog maintains the accessibility features that were previously handled by explicit ARIA attributes.
✅ Verification successful
StandardDialog implementation maintains proper accessibility features ✓
The component implements all necessary accessibility features including proper ARIA attributes, semantic structure, and keyboard interaction support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for StandardDialog implementation to verify accessibility features ast-grep --pattern 'export $_ function StandardDialog' -l typescript rg -A 10 'StandardDialog.*=.*{' --type typescriptLength of output: 571
Script:
#!/bin/bash # Find StandardDialog component file fd StandardDialog # Look for StandardDialog implementation with proper regex rg -l "StandardDialog.*=" --type typescript # Check imports in the current file cat services/main-frontend/src/components/page-specific/manage/courses/id/references/NewReferenceDialog.tsx | grep -A 5 "^import" # Search for Dialog-related components rg -l "Dialog.*=" --type typescriptLength of output: 959
Script:
#!/bin/bash # Check StandardDialog implementation cat shared-module/packages/common/src/components/StandardDialog.tsx # Look for any accessibility-related tests fd -e test.tsx -e spec.tsx StandardDialogLength of output: 3580
services/cms/src/blocks/FlipCard/FrontFlipCard/FrontFlipCardEditor.tsx (1)
20-24: Duplicate styling detected.This styling is identical to the one in BackFlipCardEditor. This duplication reinforces the need to extract the common styling into a shared constant.
services/course-material/src/components/ContentRenderer/moocfi/FlipCard/FlipButton.tsx (1)
13-16: Consider responsive positioning alternatives.Fixed positioning at the bottom right might cause issues on mobile devices or when multiple flip cards are present on the same page. Consider using absolute positioning relative to the flip card container instead.
services/cms/src/blocks/FlipCard/FlipCardEditor.tsx (1)
35-39: Third instance of duplicate styling.This is the third occurrence of the same width styling across flip card components, strongly indicating the need for a shared styling solution.
services/course-material/src/components/ContentRenderer/moocfi/FlipCard/InnerCardBlock.tsx (2)
7-7: LGTM!Clean import of the new
FlipButtoncomponent.
Line range hint
64-85: LGTM!Good improvements to the layout:
- Adding
position: relativecreates a proper positioning context- Consistent spacing and alignment rules
- Well-structured CSS for content elements
services/cms/src/blocks/FlipCard/BackFlipCard/BackFlipCardEditor.tsx (1)
1-1: LGTM! Import statement is properly organized.The addition of the Emotion CSS import is well-placed and aligns with the standardization of styling across components.
services/main-frontend/src/components/page-specific/manage/courses/id/language-versions/NewCourseLanguageVersionDialog.tsx (1)
22-32: LGTM! Clean migration to StandardDialog.The implementation correctly uses StandardDialog with proper translation and maintains all necessary functionality from the form component.
services/cms/src/styles/LocalStyles.tsx (1)
16-16: LGTM! Consistent button styling implementation.The update correctly applies the new functional button styling system while maintaining consistency with other button variants.
services/main-frontend/src/components/page-specific/manage/courses/id/course-instances/NewCourseInstanceDialog.tsx (1)
39-48: Clean implementation of StandardDialog!The transition to StandardDialog simplifies the component while maintaining all necessary functionality. The error handling and form integration are well-implemented.
services/main-frontend/src/components/page-specific/manage/courses/id/exams/EditExamDialog.tsx (1)
48-58: Well-structured StandardDialog implementation!Good practices observed:
- Clean error handling with ErrorBanner
- Proper mutation reset in onClose
- Well-organized form integration
services/main-frontend/src/components/forms/EditReferenceForm.tsx (1)
106-106: LGTM! Appropriate button styling.The change to
variant="secondary"for the delete button follows good UI patterns by visually distinguishing destructive actions from primary actions.services/main-frontend/src/components/page-specific/manage/courses/id/pages/PageList/PageList.tsx (1)
111-120: LGTM! Clean dialog implementation.The direct rendering of
NewOrEditPageFormwith proper visibility controls (open/onClose) simplifies the component hierarchy while maintaining functionality.services/main-frontend/src/components/page-specific/manage/courses/id/course-instances/NewCourseInstanceForm.tsx (1)
107-113: LGTM! Enhanced button styling.The addition of
fullWidthto the submit button improves visual hierarchy and form layout consistency.shared-module/packages/common/src/components/StandardDialog.tsx (2)
10-21: LGTM! Well-structured props interface.The props interface is comprehensive and well-typed, with good documentation through TypeScript types.
47-48: LGTM! Good accessibility implementation.Proper ARIA attributes and dynamic ID generation using
useIdhook ensure accessibility compliance.services/main-frontend/src/components/forms/NewReferenceForm.tsx (1)
94-100: LGTM! Button styling improvements.The addition of
fullWidthprop aligns with the PR's goal of standardizing form interactions across the application.services/main-frontend/src/components/page-specific/manage/courses/id/pages/NewOrEditPageForm.tsx (1)
31-33: LGTM! Well-structured interface changes.The addition of
openandonCloseprops properly implements the dialog pattern.services/main-frontend/src/components/forms/EditExamForm.tsx (1)
81-121: LGTM! Improved form layout structure.The addition of consistent margins and proper field grouping enhances readability and user experience.
system-tests/src/tests/course-language-versions.spec.ts (1)
41-41: LGTM! More descriptive success message.The updated message "Course created successfully" provides clearer feedback about the specific operation that succeeded.
services/main-frontend/src/components/page-specific/manage/courses/id/index/UpdateCourseForm.tsx (3)
22-23: LGTM! Props interface updated correctly.The new props
openandonCloseare properly typed and align with the StandardDialog component requirements.
68-79: LGTM! StandardDialog implementation looks good.The StandardDialog component is properly configured with:
- Correct title translation
- Proper button configuration
- Appropriate open/close handlers
62-62: LGTM! Proper cleanup after mutation.The
onClosehandler is correctly called after successful mutation, ensuring proper dialog cleanup.services/main-frontend/src/components/page-specific/manage/courses/id/pages/PageList/PageListItem.tsx (1)
68-80: LGTM! Form implementation properly updated.The NewOrEditPageForm is now rendered with appropriate props:
- Correct state management for visibility
- Proper cleanup on form submission
- Consistent prefix handling
services/main-frontend/src/components/forms/NewExamForm.tsx (2)
105-109: LGTM! Form layout improvements.The styled container with consistent margin improves the visual hierarchy of the form.
170-176: LGTM! Button styling properly updated.The Button component is correctly configured with fullWidth prop for consistent styling.
system-tests/src/tests/material-reference.spec.ts (2)
38-38: LGTM! Simplified wait conditions.Removed unnecessary wait for Close button, making the test more focused and reliable.
82-82: LGTM! Improved selector reliability.Changed to use semantic role selector
headingwhich is more reliable than text-based selectors.services/main-frontend/src/components/page-specific/manage/courses/id/pages/PageList/PageAudioWidget.tsx (1)
15-15: LGTM!The import statement for
StandardDialogis correctly placed with other imports from the same module.shared-module/packages/common/src/components/Button.tsx (3)
12-12: LGTM!The
fullWidthprop is correctly added as an optional boolean property to theButtonPropsinterface.
Line range hint
17-76: LGTM!The
BASE_BUTTON_STYLESfunction has been updated to handle thefullWidthprop correctly. The implementation:
- Accepts props as a parameter
- Conditionally applies full width styles
- Maintains existing button styling
Line range hint
261-265: LGTM!The
IconButtonimplementation follows the established pattern of other button variants and correctly uses both base and specific styles.system-tests/src/tests/draft.spec.ts (1)
121-121: LGTM!The success message has been consistently updated across all test cases from "Operation successful!" to "Course created successfully", providing more specific feedback to users.
Also applies to: 155-155, 187-187
services/main-frontend/src/components/page-specific/manage/courses/id/index/ManageCourse.tsx (1)
146-154: LGTM!The form visibility management has been improved:
- Clear state management using
setShowForm- Proper props passing to
UpdateCourseForm- Clean implementation of the dialog open/close behavior
services/main-frontend/src/components/page-specific/manage/courses/id/pages/ManageCourseStructure.tsx (3)
18-18: LGTM! Import of standardized dialog component.The change aligns with the initiative to standardize dialog implementations across the application.
111-113: Improve state management in handleCreateChapter.The function now explicitly sets the state to false and clears the edited chapter, which is more predictable than the previous toggle behavior.
294-305: LGTM! Standardized dialog implementation.The replacement of the custom Dialog with ChapterFormDialog:
- Improves consistency across the application
- Properly handles form state and closure
- Provides clear props interface
system-tests/src/tests/quizzes/create-and-save-quizzes.spec.ts (1)
36-36: LGTM! Updated test to match new success message.The test now correctly expects the standardized success message "Course created successfully", maintaining consistency with the localization changes.
.stylelintrc (1)
9-9: Verify the necessity of disabling at-rule descriptor validation.While this change allows for more flexible styling, it could potentially let invalid CSS through. Please confirm if this is required for specific styling patterns in the codebase.
shared-module/packages/common/src/locales/ar/main-frontend.json (1)
121-121: LGTM! Comprehensive Arabic translations added.New translations added for:
- Course creation success/error messages
- Edit/New chapter/course actions
- Page headers
These additions maintain consistency across the application's localization.Also applies to: 169-170, 193-193, 277-277, 282-282, 478-478
shared-module/packages/common/src/locales/en/main-frontend.json (1)
121-121: LGTM! English translations are clear and follow consistent conventions.The new entries maintain consistent formatting, use proper grammar, and provide clear user feedback messages.
Also applies to: 169-171, 193-195, 277-282, 478-478
shared-module/packages/common/src/locales/fi/main-frontend.json (1)
121-121: LGTM! Finnish translations are semantically accurate.The translations maintain proper Finnish grammar and are consistent with the application's terminology.
Also applies to: 171-172, 195-195, 281-286, 482-482
shared-module/packages/common/src/locales/uk/main-frontend.json (1)
122-122: LGTM! Ukrainian translations are semantically accurate.The translations maintain proper Ukrainian grammar and are consistent with the application's terminology.
Also applies to: 170-171, 194-194, 278-283, 479-479
services/course-material/src/components/ContentRenderer/moocfi/FlipCard/FlipButton.tsx
Show resolved
Hide resolved
| <ReplayArrowLeftRight size={16} /> | ||
| </div> | ||
| </Button> | ||
| <FlipButton /> |
There was a problem hiding this comment.
💡 Codebase verification
FlipButton requires accessibility improvements
The current implementation has several accessibility issues that need to be addressed:
- Add
aria-pressedattribute to indicate the current flip state - Include
aria-labelfor the icon (ReplayArrowLeftRight) - Increase the font size from 9px to meet WCAG guidelines (minimum 12px)
- Consider replacing fixed positioning to prevent overlay issues
- Add keyboard event handlers for flip functionality
- Consider adding a
prefers-reduced-motionmedia query for animations
🔗 Analysis chain
Verify FlipButton implementation and accessibility.
The FlipButton is used consistently in both image and content cases. Let's verify its implementation:
Also applies to: 85-85
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check FlipButton implementation for accessibility and consistent styling
ast-grep --pattern 'export const FlipButton: React.FC = () => {
$$$
}'
# Check for aria attributes and keyboard handlers
rg -A 5 'aria-|onKey' services/course-material/src/components/ContentRenderer/moocfi/FlipCard/FlipButton.tsx
Length of output: 185
Script:
#!/bin/bash
# Find the FlipButton file
fd -e tsx -e ts -e js -e jsx FlipButton
# If found, check its content
rg -l "FlipButton" services/course-material/src/components/ContentRenderer/moocfi/FlipCard/ --type ts --type tsx
# Get the full content of any matching files to analyze the implementation
find services/course-material/src/components/ContentRenderer/moocfi/FlipCard/ -type f -name "*FlipButton*" -exec cat {} \;
Length of output: 1553
Script:
#!/bin/bash
# Check the base Button component implementation
cat "@/shared-module/common/components/Button" || true
fd -e tsx -e ts Button shared-module/common/components/
find . -type f -name "Button.tsx" -o -name "Button.ts" | xargs cat
Length of output: 8073
| const MessageDialog: React.FC<MessageDialogProps> = ({ title, description, open, onClose }) => { | ||
| return ( | ||
| <MessageDialogContainer> | ||
| <MessageDialogTitle>{title}</MessageDialogTitle> | ||
| <MessageDialogDescription>{description}</MessageDialogDescription> | ||
| </MessageDialogContainer> | ||
| <StandardDialog open={open} onClose={onClose} title={title}> | ||
| <div | ||
| className={css` | ||
| color: #535a66; | ||
| padding: 16px 0; | ||
| `} | ||
| > | ||
| {description} | ||
| </div> | ||
| </StandardDialog> |
There was a problem hiding this comment.
Ensure 'onClose' prop is required
The onClose prop is optional in the interface but is essential for the dialog to function correctly.
Update the MessageDialogProps interface to make onClose required:
interface MessageDialogProps {
title: string
description: string
open: boolean
- onClose?: () => void
+ onClose: () => void
}Committable suggestion skipped: line range outside the PR's diff.
...src/components/exercise-service-views/ExerciseEditor/QuizComponents/common/MessageDialog.tsx
Show resolved
Hide resolved
| onSubmitForm={() => { | ||
| onSubmitForm() | ||
| onClose() | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling potential Promise rejection.
The form submission should handle potential failures to ensure the dialog doesn't close prematurely.
- onSubmitForm={() => {
- onSubmitForm()
- onClose()
- }}
+ onSubmitForm={async () => {
+ try {
+ await onSubmitForm()
+ onClose()
+ } catch (error) {
+ // Handle error or let it propagate
+ throw error
+ }
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onSubmitForm={() => { | |
| onSubmitForm() | |
| onClose() | |
| }} | |
| onSubmitForm={async () => { | |
| try { | |
| await onSubmitForm() | |
| onClose() | |
| } catch (error) { | |
| // Handle error or let it propagate | |
| throw error | |
| } | |
| }} |
nygrenh
changed the title
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Release Notes
New Features
StandardDialogcomponent for improved dialog management across the applicationFlipButtoncomponent for interactive flip card functionalityImprovements
react-hook-formin multiple componentsLocalization
Bug Fixes
Documentation