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 editor issues #4984

Merged
merged 2 commits into from
Feb 20, 2024
Merged

Fix editor issues #4984

merged 2 commits into from
Feb 20, 2024

Conversation

whoselen
Copy link
Collaborator

ISSUE

Context

Resolved editor related issues & made some changes to inbox editor with RichTextEditor component.

// Delete the below section once completed

PR Checklist

  • Description is clearly stated under Context section
  • Screenshots and the additional verifications are attached

@whoselen whoselen self-assigned this Feb 20, 2024
Copy link

sonarcloud bot commented Feb 20, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: This pull request introduces several enhancements to the RichTextEditor component and its usage within the inbox's RespondBox. It includes the addition of new hooks, refactoring from class components to functional components, and improvements in handling editor content changes. The changes aim to resolve editor-related issues, improve performance, and enhance the user experience in composing messages.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
  • Big diff: the diff is too large to approve with confidence.

General suggestions:

  • Ensure comprehensive testing of the editor's new functionalities and behavior, especially in edge cases such as rapid typing or complex content.
  • Consider documenting the newly introduced hooks and methods for future reference and for new contributors to understand their purposes easily.
  • Review the use of local storage for storing editor content to ensure it aligns with privacy policies and data handling standards.
packages/erxes-ui/src/components/richTextEditor/RichTextEditorControl/styles.ts: Consider simplifying the `FileInputAction` component by reducing nesting and avoiding absolute positioning for easier maintenance and better responsiveness.

I've noticed that the recent changes introduce a higher level of complexity, particularly with increased nesting and the use of absolute positioning within FileInputAction. While these changes might achieve the desired visual layout, they could potentially make future maintenance more challenging. Specifically, deeper nesting can make the component structure harder to follow, and absolute positioning can be fragile in responsive designs or when adjusting the layout.

To simplify, I recommend considering the following adjustments:

  • Reduce the depth of nested elements where possible. This can often be achieved with flexbox or grid layouts, which might eliminate the need for some nested structures.
  • Utilize more reusable styled components or CSS classes instead of adding multiple inline styles or direct child selectors. This approach can help reduce specificity and make the styles more modular.
  • Rethink the use of absolute positioning. If the layout can be achieved with flexbox or grid, it would likely be more resilient to content changes and easier to maintain.

Here's a simplified example of how FileInputAction might be adjusted to reduce complexity:

const FileInputAction = styled.div`
  cursor: pointer;
  background-color: #fff;
  border: 0.0625rem solid #eee;
  border-radius: 0.25rem;
  padding: 0.2rem 0.5rem;
  display: flex;
  align-items: center;
  justify-content: center;

  label {
    &:hover {
      cursor: pointer;
    }
  }

  input[type='file'] {
    display: none;
  }
`;

This approach aims to maintain readability and reduce unnecessary complexity, making the component easier to understand and maintain.

packages/erxes-ui/src/components/richTextEditor/hooks/useExtensions.ts: Consider refactoring to separate keyboard shortcut logic from the configuration array for improved maintainability and readability.

While the intention behind the changes is clear and aims to enhance functionality, the introduction of inline functions and conditionals directly within the configuration array has made the code more complex. This approach increases cognitive load, making it harder to quickly understand the configuration's structure and the specific behaviors being introduced. Additionally, embedding logic for keyboard shortcuts within the array can complicate future maintenance and testing due to potential side effects on user interaction.

A more maintainable approach could involve separating the concerns of configuring the editor and defining keyboard shortcuts. By defining the keyboard shortcuts outside the configuration array and applying them in a modular fashion, we can keep the configuration clean and focused, thus reducing complexity. Here's a suggested refactor that achieves the same outcome but in a more maintainable way:

// Define keyboard shortcuts for HardBreak in a separate function or configuration object
const hardBreakShortcuts = {
  'Shift-Enter': () => editor.commands.setHardBreak(),
  'Mod-Enter': () => false,
  'Control-Enter': () => false,
};

// Function to apply keyboard shortcuts to an extension if needed
function applyShortcuts(extension, shortcuts) {
  return extension.extend({
    addKeyboardShortcuts() {
      return shortcuts;
    },
  });
}

// Original configuration array with a cleaner approach to applying the extension
() => [
  TableImproved.configure({
    resizable: true,
  }),
  TableRow,
  TableHeader,
  TableCell,
  Document,
  BulletList,
  CodeBlock,
  applyShortcuts(HardBreak, hardBreakShortcuts), // Apply shortcuts here in a more modular way
  ListItem,
  OrderedList,
  Paragraph,
  CustomSubscript,
  CustomSuperscript,
  Text,
  Bold,
  Blockquote,
  Code,
  Italic,
]

This refactor keeps the core configuration array streamlined and enhances readability, making it easier for future developers to understand and maintain the code.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@Anu-Ujin Anu-Ujin merged commit 7d35c50 into dev Feb 20, 2024
56 of 59 checks passed
@Anu-Ujin Anu-Ujin deleted the fix-editor-issues branch February 20, 2024 18:18
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.

None yet

2 participants